-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Deprecate debug options and always use query options #8768
Conversation
pinot-core/src/main/java/org/apache/pinot/core/startree/operator/StarTreeFilterOperator.java
Outdated
Show resolved
Hide resolved
...r/src/main/java/org/apache/pinot/broker/routing/segmentselector/RealtimeSegmentSelector.java
Outdated
Show resolved
Hide resolved
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); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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: