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

feat: remove timeout in the channel between frontend and datanode #3962

Merged
merged 4 commits into from
May 16, 2024

Conversation

waynexia
Copy link
Member

@waynexia waynexia commented May 16, 2024

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

Don't timeout on gRPC connection from frontend to datanode. For some queries are expected to take a long while.

Checklist

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

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@github-actions github-actions bot added docs-not-required This change does not impact docs. and removed docs-required This change requires docs update. labels May 16, 2024
@waynexia waynexia enabled auto-merge May 16, 2024 13:03
@killme2008
Copy link
Contributor

I'm not sure if this behavior is expected. If the query request from the client has already been canceled or aborted, it doesn't make sense for the requests between the frontend and the datanode to continue processing.
Personally, I think it would be better if this parameter could be configured. By default, it can be set to a relatively large value, leaving the choice ultimately up to the user.
Of course, it would be even better to properly implement request abort and cancel functionalities.

@waynexia
Copy link
Member Author

The cancellation should always be controlled by frontend protocol layer or the client. But not timeout-ed by the channel between datanode and frontend.

One related enhancement is we don't propagate this cancellation for now. I filed a issue for this #3964

Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
Signed-off-by: Ruihang Xia <waynestxia@gmail.com>
@waynexia waynexia added this pull request to the merge queue May 16, 2024
Copy link

codecov bot commented May 16, 2024

Codecov Report

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

Project coverage is 85.11%. Comparing base (a45017a) to head (5209220).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3962      +/-   ##
==========================================
- Coverage   85.40%   85.11%   -0.30%     
==========================================
  Files         978      978              
  Lines      168271   168279       +8     
==========================================
- Hits       143717   143224     -493     
- Misses      24554    25055     +501     

Merged via the queue into GreptimeTeam:main with commit 4b03045 May 16, 2024
28 checks passed
@waynexia waynexia deleted the remove-datanode-timeout branch May 16, 2024 14:30
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.

None yet

4 participants