Rollup Search API : Searching both historical rollup and non-rollup data #901

ronnaksaxena · 2023-08-22T20:23:08Z

This feature supports searching both historical rollup and non-rollup data
To use the endpoint is GET <rollup_index>,<live_index1>, <live_index2>, <live_indexX>/_search

Currently supports:

Sum aggregations
Min aggregations
Max aggregations
Avg aggregations
Value Count aggregations

It works like this:

The type of use cases that could arise are:
Base Case: No data overlap between rollup indices and live data indices

GET /rollup-data,live-data/_search

Case 2: Overlapping Data Between 1 Rollup index and 1 Live index
Attempting to handle this use case from my design doc:

Flow for this code would look like:

Case 3: Overlapping data between multiple (aliased) live data indices
Can add a comma separated list of all index names or index alias/template
GET /rollup-data,live-data-alias/_search

CheckList:

[x ] Commits are signed per the DCO using --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.

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

bowenlan-amzn · 2023-08-24T16:35:16Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

+        return false
+    }
+    // Helper fn to avoid rewritting a rollup request an extra time
+    fun isReqeustRollupFormat(request: ShardSearchRequest): Boolean {


This function check if the request was rewritten to match the rollup index data. I do not think isRequest is descriptive enough for its purpose.

intent to say there's a typo here...

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

src/main/kotlin/org/opensearch/indexmanagement/rollup/util/RollupUtils.kt

bowenlan-amzn · 2023-08-29T18:12:51Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

+                        // Avoid infinite interceptor loop
+                        if (concreteRollupIndicesArray.isNotEmpty() && concreteLiveIndicesArray.isNotEmpty() && request.source().aggregations() != null && !isRequestRewrittenIntoBuckets(request)) {
+                            // Break apart request to remove overlapping parts
+                            breakRequestIntoBuckets(request, rollupJob!!)


Misleading name, you are adding one more bucket aggregation to the request

@bowenlan-amzn I add another aggregation but wrap the existing aggregations into a sub aggregation. Would a more appropriate name be putAggregationsIntoBuckets() ?

maybe just breakIntoBuckets

bowenlan-amzn · 2023-08-30T06:40:05Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

+            // Handle the response if it came from interceptor
+            when (response) {
+                // live index
+                is QuerySearchResult -> {


Not sure why live index is QuerySearchResult, but for rollup index is QueryFetchSearchResult, why the difference here?

I removed this. I kept it for testing by sending a request with the aggregation name as "interceptor_interval_data" I can check if the behavior in the response interceptor is correct. (The response would be a QueryFetchSearchReuslt) on one rollup index)

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

bowenlan-amzn · 2023-08-30T06:53:07Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

+            client.search(
+                maxRolledDateRequest,
+                object : ActionListener<SearchResponse> {
+                    override fun onResponse(searchResponse: SearchResponse) {
+                        maxRolledDateResponse = searchResponse
+                        latch.countDown()
+                    }
+
+                    override fun onFailure(e: Exception) {
+                        logger.error("ronsax maxLiveDate request failed ", e)
+                        latch.countDown()
+                    }
+                }
+            )


We don't want this client call to be executed for each shard response

So investigate if you can save the result of this call to a variable in this class ResponseInterceptor, and reuse for other shards response

Will look into this if I am time. I think a core modification will be needed.

getsaurabh02

Lets add unit tests at the class level to be able to verify helper/util and class methods

Signed-off-by: Ronnak Saxena <[email protected]>

…gation Signed-off-by: Ronnak Saxena <[email protected]>

Signed-off-by: Ronnak Saxena <[email protected]>

bowenlan-amzn · 2023-09-08T21:00:50Z

Now we can see this change broken data stream scenario https://github.com/opensearch-project/index-management/actions/runs/6125162603/job/16626572043?pr=901

Tests with failures:
 - org.opensearch.indexmanagement.rollup.runner.RollupRunnerIT.test metadata is created for data stream rollup job when none exists
 - org.opensearch.indexmanagement.transform.TransformRunnerIT.test transform with data stream

Signed-off-by: Ronnak Saxena <[email protected]>

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

Signed-off-by: Ronnak Saxena <[email protected]>

codecov · 2023-09-11T23:53:30Z

Codecov Report

Merging #901 (9c9d242) into feature/rollup-and-live-search-2.9 (1923003) will increase coverage by 0.31%.
The diff coverage is 78.42%.

@@                           Coverage Diff                            @@
##             feature/rollup-and-live-search-2.9     #901      +/-   ##
========================================================================
+ Coverage                                 75.88%   76.19%   +0.31%     
- Complexity                                 2899     2933      +34     
========================================================================
  Files                                       366      367       +1     
  Lines                                     16624    16946     +322     
  Branches                                   2411     2503      +92     
========================================================================
+ Hits                                      12615    12912     +297     
+ Misses                                     2623     2599      -24     
- Partials                                   1386     1435      +49

Files Changed	Coverage Δ
...nsearch/indexmanagement/rollup/util/RollupUtils.kt	`79.41% <59.32%> (-2.37%)`	⬇️
...pensearch/indexmanagement/IndexManagementPlugin.kt	`89.96% <66.66%> (-0.27%)`	⬇️
...nagement/rollup/interceptor/ResponseInterceptor.kt	`80.21% <80.21%> (ø)`
...management/rollup/interceptor/RollupInterceptor.kt	`82.81% <87.23%> (+5.04%)`	⬆️

... and 13 files with indirect coverage changes

Signed-off-by: Ronnak Saxena <[email protected]>

.github/workflows/multi-node-test-workflow.yml

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

eirsep · 2023-09-13T21:05:41Z

Can you add TODOs on the client calls made from within the shard level interceptor as this would imply for one rollup search request, we are making n more search requests where n is number of shards and that would have performance implications.

We can merge this PR and continue to work on it.

Signed-off-by: Ronnak Saxena <[email protected]>

bowenlan-amzn · 2023-09-13T23:53:30Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

-                            throw IllegalArgumentException("Rollup search must have size explicitly set to 0, but found ${request.source().size()}")
+                    val isDataStream = (request.indices().any { IndexUtils.isDataStream(it, clusterService.state()) })
+                    val shardRequestIndex = request.shardId().indexName
+                    // isRollupIndex throws an exception if the request is on a data stream


What's the exception found out here?

It is a null ptr exception when checking is the shardId indexName for a data stream is a rollup index

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

bowenlan-amzn · 2023-09-14T00:01:07Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

+        return false
+    }
+    // Helper fn to avoid rewritting a rollup request an extra time
+    fun isReqeustRollupFormat(request: ShardSearchRequest): Boolean {


intent to say there's a typo here...

bowenlan-amzn · 2023-09-14T00:04:20Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

+                    val isRollupIndex = if (isDataStream) false else isRollupIndex(shardRequestIndex, clusterService.state())
+                    val (containsRollup, rollupJob) = if (isDataStream) Pair(false, null) else originalSearchContainsRollup(request)
+                    // Only modifies rollup searches and avoids internal client calls
+                    if (containsRollup || isRollupIndex) {


Do we need || isRollupIndex here?

Yes this check is needed. The base case fails unless I check for both but maybe the logic could be cleaned up to have less if checks

bowenlan-amzn · 2023-09-14T00:07:11Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

+                        // Avoid infinite interceptor loop
+                        if (concreteRollupIndicesArray.isNotEmpty() && concreteLiveIndicesArray.isNotEmpty() && request.source().aggregations() != null && !isRequestRewrittenIntoBuckets(request)) {
+                            // Break apart request to remove overlapping parts
+                            breakRequestIntoBuckets(request, rollupJob!!)


maybe just breakIntoBuckets

bowenlan-amzn · 2023-09-14T00:11:41Z

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

+                        // Rewrite the request to fit rollup format if not already done previously
+                        if (isRollupIndex && !isReqeustRollupFormat(request)) {
+                            /* Client calls from the response interceptor require request bodies of 1,
+                            otherwise do not allow size > 0 for rollup indices
+                             */
+                            if (!canHaveSize(request) && request.source().size() != 0) {
+                                throw IllegalArgumentException(
+                                    "Rollup search must have size explicitly set to 0, " +
+                                        "but found ${request.source().size()}"
+                                )
+                            }
+                            rewriteRollupRequest(request, rollupJob!!, concreteRollupIndicesArray)
                        }


Can this be moved out from the parent if condition if (containsRollup?

No I cannot I need the concreteLiveIndices array to call the rewriteRollupRequest helper function

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/RollupInterceptor.kt

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

eirsep · 2023-09-14T00:37:48Z

.github/workflows/multi-node-test-workflow.yml

@@ -3,7 +3,7 @@ name: Multi node test workflow
 on:
  pull_request:
    branches:
-      - "*"


can we remove this after PR is approved.

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt

Signed-off-by: Ronnak Saxena <[email protected]>

ronnaksaxena · 2023-09-14T18:05:17Z

@bowenlan-amzn @eirsep Thank you guys for the feedback. I pushed a new commit with suggested changes. 👍🏽

ronnaksaxena requested review from bowenlan-amzn, getsaurabh02, lezzago, praveensameneni, xluo-aws, gaobinlong, Hailong-am, amsiglan, sbcd90, eirsep and AWSHurneyt as code owners August 22, 2023 20:23

ronnaksaxena mentioned this pull request Aug 18, 2023

POC: Response interceptor #897

Closed

bowenlan-amzn reviewed Aug 24, 2023

View reviewed changes

eirsep reviewed Aug 29, 2023

View reviewed changes

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt Outdated Show resolved Hide resolved

bowenlan-amzn changed the base branch from feature/rollup-and-live-search-2.9 to 2.9 August 29, 2023 16:09

bowenlan-amzn changed the base branch from 2.9 to feature/rollup-and-live-search-2.9 August 29, 2023 16:09

bowenlan-amzn reviewed Aug 30, 2023

View reviewed changes

ronnaksaxena changed the title ~~Case2: Overlapping Data Between 1 Rollup index and 1 Live index~~ Rollup Search API : Searching both historical rollup and non-rollup data Aug 30, 2023

getsaurabh02 reviewed Aug 31, 2023

View reviewed changes

ronnaksaxena force-pushed the case2 branch from e39ff81 to c9ca74b Compare September 5, 2023 18:59

ronnaksaxena added 10 commits September 5, 2023 13:19

added response interceptor

0ea7f53

Signed-off-by: Ronnak Saxena <[email protected]>

Base case: Query Live and Rollup data with no overlap

4e93b1f

Signed-off-by: Ronnak Saxena <[email protected]>

finished base case and added integ test

7fd394a

Signed-off-by: Ronnak Saxena <[email protected]>

added to response interceptor

f023a39

Signed-off-by: Ronnak Saxena <[email protected]>

can rewrite request to bucket pipeline

4aa4202

Signed-off-by: Ronnak Saxena <[email protected]>

trying to rewrite aggregations in a helper function

c3b5477

Signed-off-by: Ronnak Saxena <[email protected]>

able to create new aggreations but getting shardIndex is not set error

5d20241

Signed-off-by: Ronnak Saxena <[email protected]>

Can find start and end times for rollup and live index

743263b

Signed-off-by: Ronnak Saxena <[email protected]>

Handles overlap between 1 live index and 1 rollup index for sum aggre…

1f07f21

…gation Signed-off-by: Ronnak Saxena <[email protected]>

added min max aggregations and fixed intersection time calculation

b4dbc26

Signed-off-by: Ronnak Saxena <[email protected]>

ronnaksaxena added 8 commits September 6, 2023 15:53

added stars to worklow files

4ef546b

Signed-off-by: Ronnak Saxena <[email protected]>

added unit test

bf04e32

Signed-off-by: Ronnak Saxena <[email protected]>

resolved some PR comments from Bowen

d618e90

Signed-off-by: Ronnak Saxena <[email protected]>

resolved more comments on my PR

9c567ee

Signed-off-by: Ronnak Saxena <[email protected]>

removed stars from workflows

79f68e5

Signed-off-by: Ronnak Saxena <[email protected]>

testing time of alias test case

768a2bc

Signed-off-by: Ronnak Saxena <[email protected]>

boring, took too long

fb86758

Signed-off-by: Ronnak Saxena <[email protected]>

commented out last 2 tests

17df156

Signed-off-by: Ronnak Saxena <[email protected]>

ronnaksaxena added 3 commits September 8, 2023 14:04

removed all response interceptor tests

e496c57

Signed-off-by: Ronnak Saxena <[email protected]>

added one test back

8aa3f81

Signed-off-by: Ronnak Saxena <[email protected]>

fixed data stream integ tests

016d161

Signed-off-by: Ronnak Saxena <[email protected]>

eirsep reviewed Sep 11, 2023

View reviewed changes

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt Outdated Show resolved Hide resolved

eirsep reviewed Sep 11, 2023

View reviewed changes

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt Outdated Show resolved Hide resolved

eirsep reviewed Sep 11, 2023

View reviewed changes

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt Outdated Show resolved Hide resolved

ronnaksaxena added 2 commits September 11, 2023 16:13

commented out breaking tests

96603eb

Signed-off-by: Ronnak Saxena <[email protected]>

added tests back

35ee9e6

Signed-off-by: Ronnak Saxena <[email protected]>

removed countdown latch and added coroutines

06b7b02

Signed-off-by: Ronnak Saxena <[email protected]>

AWSHurneyt reviewed Sep 13, 2023

View reviewed changes

resolved comments on the PR

acf1c4b

Signed-off-by: Ronnak Saxena <[email protected]>

AWSHurneyt approved these changes Sep 13, 2023

View reviewed changes

bowenlan-amzn reviewed Sep 14, 2023

View reviewed changes

eirsep reviewed Sep 14, 2023

View reviewed changes

src/main/kotlin/org/opensearch/indexmanagement/rollup/interceptor/ResponseInterceptor.kt Outdated Show resolved Hide resolved

resolved PR comments added kotlin docs for methods

9c9d242

Signed-off-by: Ronnak Saxena <[email protected]>

eirsep approved these changes Sep 14, 2023

View reviewed changes

bowenlan-amzn merged commit 577d333 into opensearch-project:feature/rollup-and-live-search-2.9 Sep 30, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rollup Search API : Searching both historical rollup and non-rollup data #901

Rollup Search API : Searching both historical rollup and non-rollup data #901

ronnaksaxena commented Aug 22, 2023 •

edited

Loading

bowenlan-amzn Aug 24, 2023

ronnaksaxena Sep 7, 2023

bowenlan-amzn Sep 14, 2023

bowenlan-amzn Aug 29, 2023

ronnaksaxena Sep 7, 2023

bowenlan-amzn Sep 14, 2023

bowenlan-amzn Aug 30, 2023

ronnaksaxena Sep 7, 2023 •

edited

Loading

bowenlan-amzn Aug 30, 2023

ronnaksaxena Sep 7, 2023

getsaurabh02 left a comment

bowenlan-amzn commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading

eirsep commented Sep 13, 2023

bowenlan-amzn Sep 13, 2023

ronnaksaxena Sep 14, 2023

bowenlan-amzn Sep 14, 2023

bowenlan-amzn Sep 14, 2023

ronnaksaxena Sep 14, 2023

bowenlan-amzn Sep 14, 2023

bowenlan-amzn Sep 14, 2023

ronnaksaxena Sep 14, 2023

eirsep Sep 14, 2023

ronnaksaxena commented Sep 14, 2023

Rollup Search API : Searching both historical rollup and non-rollup data #901

Rollup Search API : Searching both historical rollup and non-rollup data #901

Conversation

ronnaksaxena commented Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronnaksaxena Sep 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

getsaurabh02 left a comment

Choose a reason for hiding this comment

bowenlan-amzn commented Sep 8, 2023 • edited Loading

codecov bot commented Sep 11, 2023 • edited Loading

Codecov Report

eirsep commented Sep 13, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ronnaksaxena commented Sep 14, 2023

ronnaksaxena commented Aug 22, 2023 •

edited

Loading

ronnaksaxena Sep 7, 2023 •

edited

Loading

bowenlan-amzn commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 11, 2023 •

edited

Loading