Skip to content

refactor: passing QueryContext to RegionServer#3829

Merged
killme2008 merged 6 commits intoGreptimeTeam:mainfrom
Kelvinyu1117:refactor/kelvin-add-query-context
May 7, 2024
Merged

refactor: passing QueryContext to RegionServer#3829
killme2008 merged 6 commits intoGreptimeTeam:mainfrom
Kelvinyu1117:refactor/kelvin-add-query-context

Conversation

@Kelvinyu1117
Copy link
Copy Markdown
Contributor

@Kelvinyu1117 Kelvinyu1117 commented Apr 28, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

close #3752

What's changed and what's your intention?

Allow passing the query context to the region server.

Please explain IN DETAIL what the changes are in this PR and why they are needed:

  • Summarize your change (mandatory)
  • How does this PR work? Need a brief introduction for the changed logic (optional)
  • Describe clearly one logical change and avoid lazy messages (optional)
  • Describe any limitations of the current code (optional)

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

@Kelvinyu1117 Kelvinyu1117 requested review from a team and evenyag as code owners April 28, 2024 10:53
@github-actions github-actions Bot added the docs-not-required This change does not impact docs. label Apr 28, 2024
@Kelvinyu1117 Kelvinyu1117 changed the title refactor: add QueryContext to RegionRequestHeader refactor: passing QueryContext to RegionServer Apr 28, 2024
@Kelvinyu1117 Kelvinyu1117 force-pushed the refactor/kelvin-add-query-context branch from bfb1840 to a8eff60 Compare April 28, 2024 10:59
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 28, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 85.29%. Comparing base (1b93a02) to head (9c7c2cc).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3829      +/-   ##
==========================================
- Coverage   85.61%   85.29%   -0.33%     
==========================================
  Files         952      952              
  Lines      163062   163079      +17     
==========================================
- Hits       139605   139097     -508     
- Misses      23457    23982     +525     

@Kelvinyu1117 Kelvinyu1117 force-pushed the refactor/kelvin-add-query-context branch from a8eff60 to 57142a7 Compare April 28, 2024 12:00
Comment thread src/session/src/context.rs Outdated
Comment thread src/session/src/context.rs Outdated
Copy link
Copy Markdown
Contributor

@evenyag evenyag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM.

Comment thread src/query/src/dist_plan/merge_scan.rs Outdated
Comment thread Cargo.toml Outdated
@Kelvinyu1117 Kelvinyu1117 force-pushed the refactor/kelvin-add-query-context branch from e0a2535 to 9043dd8 Compare May 2, 2024 16:20
@fengjiachun
Copy link
Copy Markdown
Collaborator

Hi @Kelvinyu1117 , there are some conflicts need to be resolved.

@killme2008
Copy link
Copy Markdown
Member

Good job! Please resolve the conflicts, then we can merge the PR. @Kelvinyu1117

@Kelvinyu1117 Kelvinyu1117 force-pushed the refactor/kelvin-add-query-context branch from 0c332cb to b8f9b4b Compare May 7, 2024 04:17
@Kelvinyu1117
Copy link
Copy Markdown
Contributor Author

Hi @Kelvinyu1117 , there are some conflicts need to be resolved.

Should be fixed.

@WenyXu
Copy link
Copy Markdown
Member

WenyXu commented May 7, 2024

Hi @Kelvinyu1117 , there are some conflicts need to be resolved.

Should be fixed.

Hi @Kelvinyu1117 , The coverage tests failed(test_do_auth_with_user_provider and test_do_auth_without_user_provider). We need to wrap the query_context with Arc in the do_auth method.

@Kelvinyu1117 Kelvinyu1117 force-pushed the refactor/kelvin-add-query-context branch from b8f9b4b to b901f82 Compare May 7, 2024 05:36
Copy link
Copy Markdown
Member

@killme2008 killme2008 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost LGTM

Comment thread src/query/src/dist_plan/merge_scan.rs
@killme2008 killme2008 enabled auto-merge May 7, 2024 06:22
Comment thread src/query/src/dist_plan/merge_scan.rs Outdated
@killme2008 killme2008 disabled auto-merge May 7, 2024 07:10
@killme2008 killme2008 merged commit 154f561 into GreptimeTeam:main May 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

docs-not-required This change does not impact docs.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pass query context down to region server service in Datanode

6 participants