Clean up BrokerRequestHandler and BrokerResponse #13179
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jackie-Jiang
added
incompatible
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #13179 +/- ##
============================================
+ Coverage 61.75% 62.21% +0.46%
+ Complexity 207 198 -9
============================================
Files 2436 2527 +91
Lines 133233 138313 +5080
Branches 20636 21394 +758
============================================
+ Hits 82274 86050 +3776
- Misses 44911 45833 +922
- Partials 6048 6430 +382
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Jackie-Jiang
force-pushed
the
broker_response
branch
2 times, most recently
from
aa1e0a7
to
ea983ce
Compare
Jackie-Jiang
force-pushed
the
broker_response
branch
from
ea983ce
to
86c9ed3
Compare
gortiz
reviewed
May 28, 2024
Comment on lines
50
to
+54
| /** | ||
| * Set the number of servers responded to the broker. | ||
| * | ||
| * @param numServersResponded number of servers responded. | ||
| * Sets the result table. We expose this method to allow modifying the results on the client side, e.g. hiding the | ||
| * results and only showing the stats. | ||
| */ | ||
| void setNumServersResponded(int numServersResponded); | ||
| void setResultTable(@Nullable ResultTable resultTable); |
There was a problem hiding this comment.
This seems to be the only mutable method in this interface. Wouldn't be better to implement the case shown in the example using a delegate/decorate pattern? Basically having an implementation that delegates all methods but getResultTable and getNumRowsResultSet to a another BrokerResponse stored as attribute
There was a problem hiding this comment.
I feel that is a little bit overkilling if the only benefit is getting rid of this setter. I'm not a big fan of this only setter as well, but I feel it is much simpler and easier to understand if we want to allow client to override it
gortiz
pushed a commit
to gortiz/pinot
that referenced
this pull request
Jun 14, 2024
mcvsubbu
added a commit
to mcvsubbu/incubator-pinot
that referenced
this pull request
Jul 18, 2024
As of PR apache#13179, the query response time metrics started to get extremely high for all tables. The root cause tured out to be a missed initialization in which the query arrival time was set to 0 by default. The refactor broke the contract that the broker request handler always returns the time taken to handle the request. Added code to initialize the arrival time. Since the refactor attempted to include client pre-processing time, added this code under the condition that the value is not already set. Ideally, callers of the method should add on their own pre and post-processing time to the time returned by the handleRequest() method.
mcvsubbu
added a commit
that referenced
this pull request
Jul 24, 2024
* Fix query response time metric As of PR #13179, the query response time metrics started to get extremely high for all tables. The root cause tured out to be a missed initialization in which the query arrival time was set to 0 by default. The refactor broke the contract that the broker request handler always returns the time taken to handle the request. Added code to initialize the arrival time. Since the refactor attempted to include client pre-processing time, added this code under the condition that the value is not already set. Ideally, callers of the method should add on their own pre and post-processing time to the time returned by the handleRequest() method. * Changed as per review comments and offline discussion * Added comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Clean up
BrokerRequestHandler:BaseBrokerRequestHandlerBaseSingleStageRequestHandlerfor the single-stage methodsClean up
BrokerResponse:BrokerResponseBrokerResponseNativeandBrokerResponseNativeV2Refactor
BrokerQueryEventListenerFactoryto be easier to use and more efficientIncompatible
BrokerResponseandBrokerRequestHandler, which might cause compile failure, but no backward incompatibility introduced