add lane enforcement for joinish queries #9563
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
|
Adding this PR for 0.18 since the laning feature is first introduced in it. |
jihoonson
reviewed
Mar 26, 2020
| @@ -462,7 +462,8 @@ public QuerySwappingQueryRunner(QueryRunner<T> baseRunner, Query<T> query, Query | |||
| throw new ISE("Unexpected query received"); | |||
| } | |||
|
|
|||
| return baseRunner.run(queryPlus.withQuery(newQuery), responseContext); | |||
| Sequence<T> result = baseRunner.run(queryPlus.withQuery(newQuery), responseContext); | |||
| return result; | |||
| { | ||
| Set<SegmentServerSelector> segmentServerSelectors = new HashSet<>(); | ||
| for (Segment s : segments) { | ||
| segmentServerSelectors.add(new SegmentServerSelector(null, s.getId().toDescriptor())); |
There was a problem hiding this comment.
Would you add @Nullable to the constructor parameter of SegmentServerSelector and getServer()?
This sounds nice, I will try to add an entry to the cluster tuning docs about this stuff before release (i guess that would be the most appropriate place) |
clintropolis
added a commit
to clintropolis/druid
that referenced
this pull request
Mar 30, 2020
* add lane enforcement for joinish queries * oops * style * review stuffs
clintropolis
added a commit
that referenced
this pull request
Mar 30, 2020
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.
Description
This PR partially squares up the joins work of #9503, #9533, and #9545 with the query laning added in #9407, by extending lane enforcement to queries that run with the
LocalQuerySegmentWalkerofClientQuerySegmentWalker, fixing a bug that queries that only use theLocalQuerySegmentWalkerare not counted at all. This PR puts things into an interim state so that these queries can fall under lane enforcement, but it has a flaw that it will sequentially acquire and release lanes for each time theClientQuerySegmentWalkerinvokes either its local or cluster walker to run a part of a query rather than acquiring all necessary lanes up front and holding through the duration of the query. This is not really an ideal state, since it opens the possibility to fail midway through if a query loses a lane to another query, but it is better than not counting them, and so worth doing for the short term I think.Ideally, we could bring up the call to
QueryScheduler.runintoClientQuerySegmentWalkeror even the http resources, but this isn't currently possible without a fairly heavy refactor of how the broker runs queries. This is due to how they are transformed on the way down, whereQueryScheduler.prioritizeAndLaneQueryhappens on in the deepest part of the cluster and local walkers runners right before they callQueryScheduler.runbecause that is when the segments participating in the query are available. I'm not quite sure what this will look like yet.This PR has:
Key changed/added classes in this PR
LocalQuerySegmentWalker