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

Deprecate debug options and always use query options #8768

Merged
merged 2 commits into from
May 26, 2022

Conversation

Jackie-Jiang
Copy link
Contributor

Currently there are 2 ways to provide extra options when running a query: query options and debug options. Most of the options are provided through the query options, and user can embed query options into the query which makes it very easy to use. On the other hand, debug options can only be provided through the query parameter of the broker GET request, which is hard to use, and sometimes impossible when querying the controller. Managing 2 sets of query options also adds confusion and extra management overhead.
This PR simplifies the query options by also including the debug options into the query options, and only look at query options when executing the query.

Release-Note

The following debug options can also be accepted as query options:

  • routingOptions
  • useScanReorderOpt

@Jackie-Jiang Jackie-Jiang added enhancement release-notes Referenced by PRs that need attention when compiling the next release notes labels May 25, 2022
@Jackie-Jiang Jackie-Jiang requested a review from KKcorps May 25, 2022 00:06
Map<String, String> debugOptions = getOptionsFromJson(jsonRequest, Broker.Request.DEBUG_OPTIONS);
if (!debugOptions.isEmpty()) {
LOGGER.debug("Debug options are set to: {} for request {}: {}", debugOptions, requestId, query);
pinotQuery.setDebugOptions(debugOptions);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not setDebugOptions at all IMO. Just putting it in query options ensures backward compatibility. We don't want other users/devs to rely on debug options for anything. Also @Deprecated annotation on setDebugOptions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do need to set it here for backward compatibility because the server might have not been upgraded to the newer version and is still expecting the option from the debug options. I didn't annotate PinotQuery.setDebugOptions() because it is auto-generated thrift class. Let me add some comments here to remove this after releasing 0.11.0

@codecov-commenter
Copy link

Codecov Report

Merging #8768 (b239724) into master (a307a23) will decrease coverage by 0.06%.
The diff coverage is 83.33%.

@@             Coverage Diff              @@
##             master    #8768      +/-   ##
============================================
- Coverage     69.72%   69.65%   -0.07%     
  Complexity     4617     4617              
============================================
  Files          1734     1736       +2     
  Lines         90992    91191     +199     
  Branches      13586    13630      +44     
============================================
+ Hits          63442    63519      +77     
- Misses        23150    23260     +110     
- Partials       4400     4412      +12     
Flag Coverage Δ
integration1 26.96% <64.28%> (-0.16%) ⬇️
integration2 25.31% <61.90%> (+0.06%) ⬆️
unittests1 66.16% <67.64%> (+<0.01%) ⬆️
unittests2 14.14% <16.66%> (-0.06%) ⬇️

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

Impacted Files Coverage Δ
...va/org/apache/pinot/spi/utils/CommonConstants.java 20.96% <0.00%> (-0.35%) ⬇️
.../org/apache/pinot/core/util/QueryOptionsUtils.java 65.00% <50.00%> (+5.00%) ⬆️
...core/startree/operator/StarTreeFilterOperator.java 88.02% <60.00%> (ø)
...inot/core/operator/filter/FilterOperatorUtils.java 87.34% <66.66%> (+1.09%) ⬆️
...roker/requesthandler/BaseBrokerRequestHandler.java 71.55% <83.33%> (+0.03%) ⬆️
...uting/segmentselector/RealtimeSegmentSelector.java 100.00% <100.00%> (+3.17%) ⬆️
...t/core/plan/AggregationGroupByOrderByPlanNode.java 81.81% <100.00%> (ø)
...rg/apache/pinot/core/plan/AggregationPlanNode.java 89.71% <100.00%> (ø)
...ava/org/apache/pinot/core/plan/FilterPlanNode.java 82.17% <100.00%> (-0.14%) ⬇️
...pinot/core/plan/maker/InstancePlanMakerImplV2.java 76.42% <100.00%> (ø)
... and 71 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a307a23...b239724. Read the comment docs.

@Jackie-Jiang Jackie-Jiang merged commit c1134a0 into apache:master May 26, 2022
@Jackie-Jiang Jackie-Jiang deleted the deprecate_debug_option branch May 26, 2022 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement release-notes Referenced by PRs that need attention when compiling the next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants