fix: refresh the rollup index as part of the rollup indexer actions #86992

salvatore-campagna · 2022-05-20T17:19:52Z

Before reading documents from the rollup index we need the index
to be refreshed to avoid the search API not hitting the expected results.
Here, instead of relying on the test to request a refresh, we let the
rollup job do the refresh of the rollup index as part of the onFinish
callback.

Resolves #81983
Resolves #53412

Using the command line provided in elastic#81983 I could not reproduce the issue. Anyway, changing the timeout from 5 seconds to just 1 second I could reproduce the issue. At the end, keeping a timeout of 1 second I could fix the issue moving the refresh operation just before running the match_all query. My understanding is that the timeout is used to give the rollup job enough time to complete the rollup operation. Anyway, other than that, after the rollup job has written data into the rollup index we need to refresh it in order for the subsequent search operation to hit the expected rolledup document.

elasticmachine · 2022-05-20T17:20:21Z

Pinging @elastic/es-analytics-geo (Team:Analytics)

elasticmachine · 2022-05-20T17:41:55Z

Pinging @elastic/clients-team (Team:Clients)

nik9000 · 2022-05-20T17:50:27Z

I genuinely unsure what's going on here. I wish we didn't have to delay some number of seconds like this. If the action is async we should be returning a task_id that we can wait_for_completion on. Is that a thing here?

salvatore-campagna · 2022-05-20T18:14:47Z

I will see in the API if we have such an option, but I think the issue here is that we might need to poll the status in the test.

csoulios

Rollup v1 indeed does not refresh the index after adding the new rollup documents.

Moving the refresh action before the search would probably fix this test. However, I was thinking that it may be preferrable to add a refresh at the end of the rollup job.

csoulios · 2022-05-26T12:41:10Z

I genuinely unsure what's going on here. I wish we didn't have to delay some number of seconds like this. If the action is async we should be returning a task_id that we can wait_for_completion on. Is that a thing here?

Rollup v1 works by scheduling a cron job (persistent task) and it's not an asyc action. So, the delay is added to make sure that the job has started and completed after job start_job has been called and the rollup job has completed.

Instead of refreshing from the yaml test we refresh once the indexing operation is complete in the callback. This reverts commit 7968c70.

salvatore-campagna · 2022-06-01T09:14:24Z

@elasticmachine update branch

csoulios

Generally the change looks good. I left some small comments

csoulios · 2022-06-07T14:06:41Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupIndexer.java

+    @Override
+    protected void onFinish(ActionListener<Void> listener) {
+        final RefreshRequest refreshRequest = new RefreshRequest(job.getConfig().getRollupIndex());
+        client.admin().indices().refresh(refreshRequest).actionGet();


What happens if the call to refresh fails? actionGet() may throw exceptions here?

Also, I think the call to refresh sould be async by calling method

client.admin().indices().refresh(RefreshRequest request, ActionListener<RefreshResponse> listener)

The big question is what happens if the call to refresh fails.
Since we got there, the index has been created successfully and we should not fail the whole job.

I guess in both cases, successful or not we can just log something.

csoulios · 2022-06-07T14:28:15Z

...ugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/RollupIndexerIndexingTests.java

@@ -705,10 +707,12 @@ private void executeTestCase(
        IndexSearcher searcher = new IndexSearcher(reader);
        String dateHistoField = config.getGroupConfig().getDateHistogram().getField();
        final ThreadPool threadPool = new TestThreadPool(getTestName());
+        final Client client = Mockito.mock(Client.class);


Perhaps you would like to use NoOpClient here and everywhere else. Although Mockito should do its job fine here, we generally prefer this as to mocking the class directly.

Ok I didn't see we had a NoOpClient. I like using it too...at least it spares us from having an additional dependency on Mockito (even if it is already there for one of the test classes).

salvatore-campagna · 2022-06-08T11:34:35Z

@elasticmachine update branch

csoulios

Maybe it's worth trying implementing this logic in ClientRollupPageManager

csoulios · 2022-06-08T12:44:53Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupIndexer.java

@@ -85,13 +98,15 @@ public abstract class RollupIndexer extends AsyncTwoPhaseIndexer<Map<String, Obj
     * @param jobStats jobstats instance for collecting stats
     */
    RollupIndexer(
+        Client client,


I was looking at the code again. RollupIndexer is an abstract class and its implementaion is ClientRollupPageManager that already has an instance of the client. So, I think the index refresh code should be implemented in the ClientRollupPageManager#onFinish. Is there any reason it should not be there?

csoulios · 2022-06-08T12:49:44Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupIndexer.java

@@ -115,6 +130,23 @@ protected void onStart(long now, ActionListener<Boolean> listener) {
        }
    }

+    @Override
+    protected void onFinish(ActionListener<Void> listener) {


At this point, I think you should at least invoke the listener.onResponse(null) method.

Alternatively, you can see the code at TransformIndexer for inspiration.

csoulios · 2022-06-08T13:28:06Z

Also, this PR fixes #53412

salvatore-campagna · 2022-06-09T11:19:15Z

@elasticmachine update branch

The client is used to call the refresh action too. As a result, using 'any()' as the action would match both the search and refresh actions while we just need to handle the search action.

csoulios

Left some more comments. Basically, now that refresh happens in ClientRollupPageManager, we don't need the client dependency in the RollupIndexer class and all its subclasses.

csoulios · 2022-06-24T09:37:07Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupIndexer.java

@@ -61,6 +62,7 @@
 public abstract class RollupIndexer extends AsyncTwoPhaseIndexer<Map<String, Object>, RollupIndexerJobStats> {
    static final String AGGREGATION_NAME = RollupField.NAME;

+    private final Client client;


Since now we use the ClientRollupPageManager implementation to refresh the rollup index,
client instance does not need to be injected any more.

csoulios · 2022-06-24T09:39:36Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupJobTask.java

+
+                @Override
+                public void onResponse(RefreshResponse refreshResponse) {
+                    logger.info("refreshing rollup index {} successful for job {}", jobConfig.getRollupIndex(), jobConfig.getId());


Maybe this should be logger.trace()? It does not look very important information so that its logged at info level

csoulios · 2022-06-24T09:42:44Z

x-pack/plugin/rollup/src/main/java/org/elasticsearch/xpack/rollup/job/RollupJobTask.java

+                    );
+                }
+            };
+            client.admin().indices().refresh(new RefreshRequest(jobConfig.getRollupIndex()), refreshResponseActionListener);
            listener.onResponse(null);


listener#onResponse should probably be called from inside
refreshResponseActionListener#onResponse and refreshResponseActionListener#onFailure so that
the responses of the async calls are chained.

Doing this breaks tests in RollupJobTasks like testTriggerWithHeaders on

assertBusy(() -> assertTrue(finished.get()));

csoulios · 2022-06-24T09:59:39Z

.../plugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/RollupIndexerStateTests.java

@@ -53,22 +56,24 @@
 public class RollupIndexerStateTests extends ESTestCase {
    private static class EmptyRollupIndexer extends RollupIndexer {
        EmptyRollupIndexer(
+            Client client,


EmptyRollupIndexer and DelayedEmptyRollupIndexer probably don't need a client dependency

salvatore-campagna · 2022-07-05T09:21:46Z

@elasticmachine update branch

csoulios

Generally, change looks good.

I have only left a comment that the client arg should be removed from the SyncRollupIndexer. I don't think Client is used in this class. After changing this, no other review is required.

Thank you for fixing this bug.

csoulios · 2022-07-05T10:14:44Z

...ugin/rollup/src/test/java/org/elasticsearch/xpack/rollup/job/RollupIndexerIndexingTests.java

@@ -819,6 +821,7 @@ class SyncRollupIndexer extends RollupIndexer {
        private Exception exc;

        SyncRollupIndexer(
+            Client client,


I do not think the client constructor parameter is used anywhere. Perhaps you should remove it

We also need to mock the refresh response for the client to actually propagate the result of the refresh call.

csoulios · 2022-07-06T09:49:20Z

Yeah! Thanks a lot for fixing this, Salvatore.

Test had been muted because of issue #67749 This PR unmutes test RollupIT.testPutStartAndGetRollupJob() because most probably it has been fixed by #86992 Closes #67749

salvatore-campagna requested a review from csoulios May 20, 2022 17:20

elasticsearchmachine added the v8.3.0 label May 20, 2022

salvatore-campagna added >bug :StorageEngine/Rollup Turn fine-grained time-based data into coarser-grained data and removed v8.3.0 labels May 20, 2022

elasticmachine added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 20, 2022

salvatore-campagna added >test-failure Triaged test failures from CI v8.3.0 and removed Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) >bug labels May 20, 2022

sethmlarson added the Team:Clients Meta label for clients team label May 20, 2022

salvatore-campagna self-assigned this May 25, 2022

craigtaverner added v8.4.0 and removed v8.3.0 labels May 25, 2022

csoulios reviewed May 26, 2022

View reviewed changes

salvatore-campagna added 2 commits May 31, 2022 15:54

fix: refresh the rollup index when rollup is done

665647d

Revert "fix: move refresh just before the match_all query"

d3e9dd1

Instead of refreshing from the yaml test we refresh once the indexing operation is complete in the callback. This reverts commit 7968c70.

salvatore-campagna requested a review from csoulios June 1, 2022 08:17

Merge branch 'master' into fix/81983-security-tests-rollup

6385514

salvatore-campagna changed the title ~~fix: move refresh just before the match_all query~~ fix: refresh the rollup index as part of the rollup indexer actions Jun 1, 2022

csoulios reviewed Jun 7, 2022

View reviewed changes

salvatore-campagna added 2 commits June 8, 2022 11:11

fix: log refresh failure

f48593c

use NoOpClient in place of Mockito mock

ea442d5

salvatore-campagna requested a review from csoulios June 8, 2022 09:22

fix: remove redundant 'final' modifier

2688bb1

Merge branch 'master' into fix/81983-security-tests-rollup

8e11d8c

csoulios reviewed Jun 8, 2022

View reviewed changes

fix: move 'onFinish' to subclass

a46af31

elasticmachine and others added 2 commits June 9, 2022 12:19

Merge branch 'master' into fix/81983-security-tests-rollup

f9bbc68

fix: just mock the search action call

691c54a

The client is used to call the refresh action too. As a result, using 'any()' as the action would match both the search and refresh actions while we just need to handle the search action.

salvatore-campagna requested a review from csoulios June 15, 2022 08:55

csoulios reviewed Jun 24, 2022

View reviewed changes

salvatore-campagna added 2 commits July 5, 2022 11:06

fix: remove unnecessary client dependency

3bd3a94

fix: call onResponse from inside and change logging level

c1f70f3

salvatore-campagna requested a review from csoulios July 5, 2022 09:07

elasticmachine and others added 2 commits July 5, 2022 10:21

Merge branch 'master' into fix/81983-security-tests-rollup

4e5fc42

fix: move back onResponse call

16bd5b0

csoulios approved these changes Jul 5, 2022

View reviewed changes

salvatore-campagna added 2 commits July 5, 2022 14:10

fix: remove client dependency

24ee30a

fix: move listener callback into the refresh listener

ceb5c6b

We also need to mock the refresh response for the client to actually propagate the result of the refresh call.

salvatore-campagna merged commit 6aca35c into elastic:master Jul 6, 2022

csoulios mentioned this pull request Sep 9, 2022

[7.17] Unmute RollupIT.testPutStartAndGetRollupJob() test #89979

Merged

csoulios added a commit that referenced this pull request Sep 12, 2022

Unmute test (#89979)

e85056b

Test had been muted because of issue #67749 This PR unmutes test RollupIT.testPutStartAndGetRollupJob() because most probably it has been fixed by #86992 Closes #67749

csoulios mentioned this pull request Sep 12, 2022

RollupIT#testPutStartAndGetRollupJob failure #67749

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: refresh the rollup index as part of the rollup indexer actions #86992

fix: refresh the rollup index as part of the rollup indexer actions #86992

salvatore-campagna commented May 20, 2022 •

edited

Loading

elasticmachine commented May 20, 2022

elasticmachine commented May 20, 2022

nik9000 commented May 20, 2022

salvatore-campagna commented May 20, 2022 •

edited

Loading

csoulios left a comment •

edited

Loading

csoulios commented May 26, 2022

salvatore-campagna commented Jun 1, 2022

csoulios left a comment

csoulios Jun 7, 2022

salvatore-campagna Jun 8, 2022

csoulios Jun 7, 2022

salvatore-campagna Jun 8, 2022

salvatore-campagna commented Jun 8, 2022

csoulios left a comment

csoulios Jun 8, 2022

csoulios Jun 8, 2022

csoulios commented Jun 8, 2022

salvatore-campagna commented Jun 9, 2022

csoulios left a comment

csoulios Jun 24, 2022

csoulios Jun 24, 2022

csoulios Jun 24, 2022

salvatore-campagna Jul 5, 2022

csoulios Jun 24, 2022

salvatore-campagna commented Jul 5, 2022

csoulios left a comment •

edited

Loading

csoulios Jul 5, 2022

csoulios commented Jul 6, 2022

fix: refresh the rollup index as part of the rollup indexer actions #86992

fix: refresh the rollup index as part of the rollup indexer actions #86992

Conversation

salvatore-campagna commented May 20, 2022 • edited Loading

elasticmachine commented May 20, 2022

elasticmachine commented May 20, 2022

nik9000 commented May 20, 2022

salvatore-campagna commented May 20, 2022 • edited Loading

csoulios left a comment • edited Loading

Choose a reason for hiding this comment

csoulios commented May 26, 2022

salvatore-campagna commented Jun 1, 2022

csoulios left a 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

salvatore-campagna commented Jun 8, 2022

csoulios left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios commented Jun 8, 2022

salvatore-campagna commented Jun 9, 2022

csoulios left a 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

salvatore-campagna commented Jul 5, 2022

csoulios left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csoulios commented Jul 6, 2022

salvatore-campagna commented May 20, 2022 •

edited

Loading

salvatore-campagna commented May 20, 2022 •

edited

Loading

csoulios left a comment •

edited

Loading

csoulios left a comment •

edited

Loading