-
Notifications
You must be signed in to change notification settings - Fork 111
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
Allowing non-rollup and rollup indices to be searched together #1268
base: main
Are you sure you want to change the base?
Allowing non-rollup and rollup indices to be searched together #1268
Conversation
Signed-off-by: Kshitij Tandon <[email protected]>
4ea022d
to
5e9c1c5
Compare
Signed-off-by: Kshitij Tandon <[email protected]>
thanks @tandonks for taking this change up! LGTM |
Thanks. What would be the condition to use this? Better to provide the instructions for normal users on when they can enable this and search source and target. |
@@ -1092,12 +1092,12 @@ class RollupInterceptorIT : RollupRestTestCase() { | |||
}, | |||
"aggs": { | |||
"sum_passenger_count": { "sum": { "field": "passenger_count" } }, | |||
"max_passenger_count": { "max": { "field": "passenger_count" } }, | |||
"value_count_passenger_count": { "value_count": { "field": "passenger_count" } } |
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.
why removing value_count?
It's better to check all this values are right in rollup search
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.
It was initially also not being used in the test earlier since we were testing the validation (where we used to throw the exception) on rollup and source index searches together
I was actually trying with that too but was getting an Internal Server Error for the same
{"error":{"root_cause":[],"type":"search_phase_execution_exception","reason":"","phase":"fetch","grouped":true,"failed_shards":[],"caused_by":{"type":"class_cast_exception","reason":"class org.opensearch.search.aggregations.metrics.InternalScriptedMetric cannot be cast to class org.opensearch.search.aggregations.metrics.InternalValueCount (org.opensearch.search.aggregations.metrics.InternalScriptedMetric and org.opensearch.search.aggregations.metrics.InternalValueCount are in unnamed module of loader 'app')"}},"status":500}
@@ -347,6 +353,9 @@ class RollupInterceptor( | |||
if (searchAllJobs) { | |||
request.source(request.source().rewriteSearchSourceBuilder(matchingRollupJobs.keys, fieldNameMappingTypeMap, concreteSourceIndex)) | |||
} else { | |||
if (matchingRollupJobs.keys.size > 1) { | |||
logger.warn("Trying search with search across multiple rollup jobs disabled so will give result with largest rollup window") |
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.
Let move this to trace since multiple such search queries can lead to significant logging.
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.
Done
Signed-off-by: Kshitij Tandon <[email protected]>
Description
Introduced a new setting ROLLUP_SEARCH_SOURCE_INDICES which when set to true allows users to search non-rollup and rollup indices together.
It's just a way to perform searches together of non-rollup and rollup indices. In order to use it, users need to update the cluster settings and set the above setting to true.
Related Issues
Resolves #1213
Check List
--signoff
.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.