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

refine query cancel resp msg #9242

Merged
merged 3 commits into from
Aug 24, 2022

Conversation

klsince
Copy link
Contributor

@klsince klsince commented Aug 18, 2022

This PR mainly refines the query cancel resp msg, by defining some explicit query cancel error code and msg, instead of bubbling up the Executor's CancellationException.

Also moves the query tracking mechanism for query cancellation from QueryScheduler to InstanceRequestHandler to be less likely to be overwritten by subclasses, and be more consistent with the logic in BaseBrokerRequestHandler.

@codecov-commenter
Copy link

codecov-commenter commented Aug 18, 2022

Codecov Report

Merging #9242 (3c5d0e9) into master (022fd37) will decrease coverage by 2.82%.
The diff coverage is 57.14%.

❗ Current head 3c5d0e9 differs from pull request most recent head d4e2c4b. Consider uploading reports for the commit d4e2c4b to get more accurate results

@@             Coverage Diff              @@
##             master    #9242      +/-   ##
============================================
- Coverage     69.93%   67.10%   -2.83%     
+ Complexity     5009     4816     -193     
============================================
  Files          1860     1382     -478     
  Lines         99185    72022   -27163     
  Branches      15082    11533    -3549     
============================================
- Hits          69367    48333   -21034     
+ Misses        24905    20176    -4729     
+ Partials       4913     3513    -1400     
Flag Coverage Δ
integration1 ?
integration2 ?
unittests1 67.10% <57.14%> (+0.01%) ⬆️
unittests2 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...che/pinot/core/query/scheduler/QueryScheduler.java 71.81% <ø> (-10.06%) ⬇️
...he/pinot/core/transport/ChannelHandlerFactory.java 50.00% <0.00%> (-50.00%) ⬇️
...va/org/apache/pinot/spi/utils/CommonConstants.java 27.69% <ø> (ø)
...core/query/executor/ServerQueryExecutorV1Impl.java 66.07% <22.22%> (-16.91%) ⬇️
...e/pinot/spi/exception/QueryCancelledException.java 33.33% <33.33%> (ø)
...e/pinot/core/transport/InstanceRequestHandler.java 60.15% <55.55%> (+0.40%) ⬆️
.../apache/pinot/common/exception/QueryException.java 93.18% <100.00%> (-1.01%) ⬇️
...not/core/operator/combine/BaseCombineOperator.java 88.31% <100.00%> (+0.31%) ⬆️
...rator/combine/SelectionOrderByCombineOperator.java 45.16% <100.00%> (-10.02%) ⬇️
...va/org/apache/pinot/core/plan/CombinePlanNode.java 85.24% <100.00%> (-2.89%) ⬇️
... and 726 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@klsince klsince force-pushed the refine_cancel_resp_msg branch 2 times, most recently from 055950a to 7d0f7ac Compare August 19, 2022 18:02
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

We also need to handle the exception in SelectionOrderByCombineOperator

@klsince klsince force-pushed the refine_cancel_resp_msg branch 2 times, most recently from 9d282c3 to d7af8d1 Compare August 22, 2022 04:47
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@klsince klsince force-pushed the refine_cancel_resp_msg branch 6 times, most recently from deebbef to 8948cb4 Compare August 23, 2022 20:20
@Jackie-Jiang Jackie-Jiang merged commit 4f95945 into apache:master Aug 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants