Batch add index block cluster state updates by joegallo · Pull Request #84374 · elastic/elasticsearch

joegallo · 2022-02-24T22:35:52Z

Related to #81627 and #83432, followup to #83760 and #84259.

elasticmachine · 2022-02-24T22:35:56Z

Pinging @elastic/es-data-management (Team:Data Management)

elasticsearchmachine · 2022-02-24T22:36:18Z

Hi @joegallo, I've created a changelog YAML for you.

DaveCTurner

Looks good except one slip that I think indicates a lack of test coverage (and a couple of tiny nits)

server/src/main/java/org/elasticsearch/cluster/metadata/MetadataIndexStateService.java

The rest of the various Executors here are not static, because they use the clusterService or some other field of the enclosing MetadataIndexStateService, but this one doesn't happen to do that, so there's no harm in making it static.

DaveCTurner

That's what I had in mind, yes. Added some small suggestions. Also could we add a cluster state listener in each case so we can assert how many cluster states get published?

...src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceBatchingTests.java

joegallo · 2022-03-04T21:13:13Z

Also could we add a cluster state listener in each case so we can assert how many cluster states get published?

It looks to me like shard-started tasks end up making this a little harder than it might initially sound -- I think they're not consistently batching in a way that makes this amenable to the super simple "just count all changed states".

Let's catch up on Monday and see if there's something we can do to be more clever and get around that.

DaveCTurner · 2022-03-05T09:27:57Z

Oh right yes that's not going to work. How about assertions that there are never any partially-completed states?

diff --git a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceBatchingTests.java b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceBatchingTests.java
index b52eae61e5e..63c7188d7a3 100644
--- a/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceBatchingTests.java
+++ b/server/src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceBatchingTests.java
@@ -10,6 +10,7 @@ package org.elasticsearch.cluster.metadata;
 
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.cluster.ClusterState;
+import org.elasticsearch.cluster.ClusterStateListener;
 import org.elasticsearch.cluster.ClusterStateTaskConfig;
 import org.elasticsearch.cluster.ClusterStateTaskListener;
 import org.elasticsearch.cluster.metadata.IndexMetadata.APIBlock;
@@ -30,6 +31,7 @@ import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcke
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.hasSize;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.oneOf;
 
 public class MetadataIndexStateServiceBatchingTests extends ESSingleNodeTestCase {
 
@@ -60,6 +62,9 @@ public class MetadataIndexStateServiceBatchingTests extends ESSingleNodeTestCase
         assertAcked(client().admin().indices().prepareClose(closedIndices));
         ensureGreen("test-1", "test-2", "test-3");
 
+        final var assertingListener = closedIndexCountListener(closedIndices.length);
+        clusterService.addListener(assertingListener);
+
         final var block1 = blockMasterService(masterService);
         block1.run(); // wait for block
 
@@ -81,12 +86,17 @@ public class MetadataIndexStateServiceBatchingTests extends ESSingleNodeTestCase
             final var indexMetadata = clusterService.state().metadata().indices().get(index);
             assertThat(indexMetadata.getState(), is(State.OPEN));
         }
+
+        clusterService.removeListener(assertingListener);
     }
 
     public void testBatchCloseIndices() throws Exception {
         final var clusterService = getInstanceFromNode(ClusterService.class);
         final var masterService = clusterService.getMasterService();
 
+        final var assertingListener = closedIndexCountListener(3);
+        clusterService.addListener(assertingListener);
+
         // create some indices
         createIndex("test-1", client().admin().indices().prepareCreate("test-1"));
         createIndex("test-2", client().admin().indices().prepareCreate("test-2"));
@@ -130,12 +140,17 @@ public class MetadataIndexStateServiceBatchingTests extends ESSingleNodeTestCase
             final var indexMetadata = clusterService.state().metadata().indices().get(index);
             assertThat(indexMetadata.getState(), is(State.CLOSE));
         }
+
+        clusterService.removeListener(assertingListener);
     }
 
     public void testBatchBlockIndices() throws Exception {
         final var clusterService = getInstanceFromNode(ClusterService.class);
         final var masterService = clusterService.getMasterService();
 
+        final var assertingListener = blockedIndexCountListener();
+        clusterService.addListener(assertingListener);
+
         // create some indices
         createIndex("test-1", client().admin().indices().prepareCreate("test-1"));
         createIndex("test-2", client().admin().indices().prepareCreate("test-2"));
@@ -179,6 +194,8 @@ public class MetadataIndexStateServiceBatchingTests extends ESSingleNodeTestCase
             final var indexMetadata = clusterService.state().metadata().indices().get(index);
             assertThat(INDEX_BLOCKS_WRITE_SETTING.get(indexMetadata.getSettings()), is(true));
         }
+
+        clusterService.removeListener(assertingListener);
     }
 
     private static CheckedRunnable<Exception> blockMasterService(MasterService masterService) {
@@ -200,6 +217,17 @@ public class MetadataIndexStateServiceBatchingTests extends ESSingleNodeTestCase
         return () -> executionBarrier.await(10, TimeUnit.SECONDS);
     }
 
+    private static ClusterStateListener closedIndexCountListener(int closedIndices) {
+        return event -> assertThat(event.state().metadata().getConcreteAllClosedIndices().length, oneOf(0, closedIndices));
+    }
+
+    private static ClusterStateListener blockedIndexCountListener() {
+        return event -> assertThat(
+            event.state().metadata().stream().filter(indexMetadata -> INDEX_BLOCKS_WRITE_SETTING.get(indexMetadata.getSettings())).count(),
+            oneOf(0L, 3L)
+        );
+    }
+
     /**
      * Listener that asserts it does not fail.
      */

That is, this is checking that we're *actually* batching these operations

joegallo · 2022-03-07T22:13:29Z

👍, @DaveCTurner I like that approach. Added as a44f745.

DaveCTurner

LGTM

joegallo added 4 commits February 24, 2022 17:33

Switch to listener.delegateFailure(...)

f049bba

Whitespace

a3d874d

Extract an executor for adding blocks

2b47e6c

Do the same for finalizing blocks

d0f3cce

joegallo added >enhancement :Data Management/Indices APIs DO NOT USE. Use ":Distributed/Indices APIs" or ":StorageEngine/Templates" instead. v8.2.0 labels Feb 24, 2022

joegallo requested review from DaveCTurner and original-brownbear February 24, 2022 22:35

elasticmachine added the Team:Data Management (obsolete) DO NOT USE. This team no longer exists. label Feb 24, 2022

Update docs/changelog/84374.yaml

3aace97

joegallo mentioned this pull request Feb 24, 2022

Batch cluster state tasks for index opening, closing, and blocks #83432

Closed

Merge branch 'master' into batch-add-blocks

fc83e82

DaveCTurner reviewed Feb 28, 2022

View reviewed changes

joegallo added 7 commits February 28, 2022 16:21

Rename this variable, oops

389de77

Make this class static

035e71a

The rest of the various Executors here are not static, because they use the clusterService or some other field of the enclosing MetadataIndexStateService, but this one doesn't happen to do that, so there's no harm in making it static.

Merge branch 'master' into batch-add-blocks

2f0413b

Merge branch 'master' into batch-add-blocks

3d2ba2c

Merge branch 'master' into batch-add-blocks

f7de377

Add a failing test

f90215b

Make it pass

227c626

joegallo requested a review from DaveCTurner March 3, 2022 20:23

DaveCTurner reviewed Mar 3, 2022

View reviewed changes

...src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceBatchingTests.java Outdated Show resolved Hide resolved

joegallo added 4 commits March 3, 2022 17:20

Drop some unnecessary assertBusy blocks

4137dca

Switch to CheckedRunnable

bb38395

Test opens against *closed* indices

7d8cbb9

Check post conditions

2762ad6

Merge branch 'master' into batch-add-blocks

72cd034

DaveCTurner reviewed Mar 4, 2022

View reviewed changes

...src/test/java/org/elasticsearch/cluster/metadata/MetadataIndexStateServiceBatchingTests.java Outdated Show resolved Hide resolved

joegallo added 2 commits March 4, 2022 15:57

Move the ensureGreen, drop the assertBusy

69d9f72

Randomly close some indices before testing opens

de90428

joegallo added 2 commits March 7, 2022 17:06

Add listeners to assert invariants

a44f745

That is, this is checking that we're *actually* batching these operations

Merge branch 'master' into batch-add-blocks

7e7af10

joegallo requested a review from DaveCTurner March 7, 2022 22:13

DaveCTurner approved these changes Mar 8, 2022

View reviewed changes

joegallo merged commit f114cc9 into elastic:master Mar 8, 2022

joegallo deleted the batch-add-blocks branch March 8, 2022 14:57

joegallo mentioned this pull request Mar 8, 2022

Batch add-block-index-to-close, add-index-block and finalize-index-block tasks #81627

Closed

Conversation

joegallo commented Feb 24, 2022

Uh oh!

elasticmachine commented Feb 24, 2022

Uh oh!

elasticsearchmachine commented Feb 24, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

joegallo commented Mar 4, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DaveCTurner commented Mar 5, 2022

Uh oh!

joegallo commented Mar 7, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

joegallo commented Mar 4, 2022 •

edited

Loading