Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: passing QueryContext to RegionServer #3829

Merged

Conversation

Kelvinyu1117
Copy link
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 evenyag and a team 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
Copy link

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
src/session/src/context.rs Outdated Show resolved Hide resolved
src/session/src/context.rs Outdated Show resolved Hide resolved
Copy link
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.

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

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

@killme2008
Copy link
Contributor

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
Contributor Author

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

Should be fixed.

@WenyXu
Copy link
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
Contributor

@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

src/query/src/dist_plan/merge_scan.rs Show resolved Hide resolved
@killme2008 killme2008 enabled auto-merge May 7, 2024 06:22
@killme2008 killme2008 disabled auto-merge May 7, 2024 07:10
@killme2008 killme2008 merged commit 154f561 into GreptimeTeam:main May 7, 2024
14 of 15 checks passed
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