-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Fix deadlock that can occur while merging group by results #15420
Changes from 30 commits
de216d5
35bf551
09689e9
e85f4b0
ce89fdf
a34bcde
a4f476d
fe32796
6ecd555
b168ce9
6cd8aa0
ac44edb
d2886bc
ceef4f1
75aee0a
951dce2
3c1b124
8c72a92
bcfabf8
3c7fd81
e110662
14f5bab
d956e73
bf57bcc
2c3e675
dc2aa53
430f2e5
76bc024
349e8d9
d26f7fc
b3850df
8f255d7
6980d77
e2da72a
9271043
b8eada8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -591,6 +591,10 @@ public boolean isWindowingStrictValidation() | |
); | ||
} | ||
|
||
public String getQueryResourceId() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO it'd be good to make this a wrapper around String like |
||
{ | ||
return getString(QueryContexts.QUERY_RESOURCE_ID); | ||
} | ||
|
||
public String getBrokerServiceName() | ||
{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -118,6 +118,22 @@ public QueryRunner<ResultType> mergeResults(QueryRunner<ResultType> runner) | |
return new ResultMergeQueryRunner<>(runner, this::createResultComparator, this::createMergeFn); | ||
} | ||
|
||
/** | ||
* Like {@link #mergeResults(QueryRunner)}, but additional context parameter to determine whether the input runner | ||
* to the method would be the result from the corresponding {@link QueryRunnerFactory#mergeRunners}. Merging can | ||
* require additional resources, like merge buffers for group-by queries, therefore the flag, can help | ||
* determine if the mergeResults should acquire those resources for the merging runners, before beginning execution. | ||
* If not overridden, this method will ignore the {@code willMergeRunner} parameter. | ||
* | ||
* Ideally {@link #mergeResults(QueryRunner)} should have delegated to this method after setting the default value of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couple questions about this:
I'm hoping we can make this |
||
* the `willMergeRunner` however this was added later, therefore the existing toolchests (in extensions) would | ||
* not have implemented this. | ||
*/ | ||
public QueryRunner<ResultType> mergeResults(QueryRunner<ResultType> runner, boolean willMergeRunner) | ||
{ | ||
return mergeResults(runner); | ||
} | ||
|
||
/** | ||
* Creates a merge function that is used to merge intermediate aggregates from historicals in broker. This merge | ||
* function is used in the default {@link ResultMergeQueryRunner} provided by | ||
|
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.
Please add javadoc for what
willMergeRunner
means. I recognize most other stuff in here doesn't have javadocs, but, still.