Batch Index Settings Update Requests by joegallo · Pull Request #82896 · elastic/elasticsearch

joegallo · 2022-01-20T21:11:19Z

Mostly follows the same shape as the changes in #82159.

~~WIP because this currently fails ./gradlew ':server:test' --tests "org.elasticsearch.indices.cluster.IndicesClusterStateServiceRandomUpdatesTests.testRandomClusterStateUpdates" pretty reliably. 😬~~

original-brownbear

Looks good, just one quick comment for now. We do need to figured out how/why this breaks tests though.

original-brownbear · 2022-01-24T14:20:33Z

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

+                }
+            }
+            // reroute in case things change that require it (like number of replicas)
+            state = allocationService.reroute(state, "settings update");


let's skip this in case the batch was a NOOP and currentState == state

👍, be4de6c

It's the first thing the reroute call itself actually called, so we're keeping a portion of a full reroute call here, but deferring the rest of it until the end of the executor loop.

elasticmachine · 2022-01-24T20:20:10Z

Pinging @elastic/es-distributed (Team:Distributed)

original-brownbear · 2022-01-25T14:07:04Z

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

-                        .build();
+                // we need to tweak auto expand replicas in order to avoid tripping assertions in
+                // AllocationService.reroute(RoutingAllocation allocation) -- this is far from ideal
+                updatedState = allocationService.adaptAutoExpandReplicas(updatedState);


Just to share the discussion we had on this:
This may be something we want to fix in some form. It's somewhat weird that reroute forces an assertion on us that ensure that this was called. Maybe this logic should be part of the settings update itself somehow so we don't even have to grind through all the indices here to check if any of them require an update for auto-expand-replicas.

I think the problem is here:

elasticsearch/server/src/test/java/org/elasticsearch/indices/cluster/ClusterStateChanges.java

Lines 468 to 478 in 6959203

private ClusterState executeClusterStateUpdateTask(ClusterState state, Runnable runnable) {

ClusterState[] result = new ClusterState[1];

doAnswer(invocationOnMock -> {

ClusterStateUpdateTask task = (ClusterStateUpdateTask) invocationOnMock.getArguments()[1];

result[0] = task.execute(state);

return null;

}).when(clusterService).submitStateUpdateTask(anyString(), any(ClusterStateUpdateTask.class), any());

runnable.run();

assertThat(result[0], notNullValue());

return result[0];

}

In these tests we run each individual ClusterStateUpdateTask but with this change we moved the tidy-up reroute() call outside the individual tasks so it's not running.

Great catch! I think 92bc39d should do the trick then, yes?

original-brownbear

LGTM, I'd like David to have a look at this too with the reroute weirdness. Maybe he has a better idea on how to cleanly handle this.

elasticsearchmachine · 2022-01-25T16:45:31Z

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

DaveCTurner

LGTM

joegallo · 2022-01-25T21:25:56Z

I opened #83097 for some of the test failures I saw here. (edit: and the fix for that has already been PRed and merged, what a world!)

* upstream/master: (762 commits) [DOCS] Add note to that log4j customization is outside the support scope (elastic#82668) Batch Index Settings Update Requests (elastic#82896) [DOCS] Delete pipeline containing stored script (elastic#83102) Try again to fix changelog areas after reorg (elastic#83100) Bind to non-localhost for transport in some cases (elastic#82973) [DOCS] Reuse multi-level `join` warning (elastic#82976) Remove unnecessary CopyOnWriteHashMap class (elastic#83040) Adjust changelog categories after reorg (elastic#83087) [DOCS] Fix typo in `action.destructive_requires_name` breaking change (elastic#83085) Stack Monitoring: Add Enterprise Search monitoring index templates (elastic#82743) [DOCS] Fix stored script example snippet (elastic#83056) [DOCS] Re-add network traffic para to `term` query (elastic#83047) [DOCS] Rename example stored script (elastic#83054) [ML][DOCS] Add Trained model APIs to the REST APIs index (elastic#82791) [ML] Update running process when global calendar changes (elastic#83044) [Transform] Fix condition on which the transform stops processing buckets (elastic#82852) [DOCS] Fixes field names in ML sum functions. (elastic#83048) [ML] fix NLP tokenization never_split handling around punctuation (elastic#82982) Construct dynamic updates directly via object builders (elastic#81449) Emit trace.id into audit logs (elastic#82849) ... # Conflicts: # client/rest-high-level/src/test/java/org/elasticsearch/client/IndicesClientIT.java # client/rest-high-level/src/test/java/org/elasticsearch/client/documentation/ILMDocumentationIT.java # server/src/main/java/org/elasticsearch/action/admin/indices/rollover/Condition.java # server/src/test/java/org/elasticsearch/action/admin/indices/rollover/ConditionTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/RolloverActionTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/TimeseriesLifecycleTypeTests.java # x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/ilm/WaitForRolloverReadyStepTests.java

joegallo added 6 commits January 20, 2022 15:01

Whitespace

2edfaa7

Assign these in constructor argument order

b93782f

Just use metadata() here

11ca164

Extract a clusterTask to pass, rather than on-the-fly

82fab8c

Move the reroute *slightly* later

2c0c648

Use a reroute-after-tasks-finished executor

86b0913

joegallo added >enhancement :Distributed/Cluster Coordination Cluster formation and cluster state publication, including cluster membership and fault detection. v8.0.0 v8.1.0 labels Jan 20, 2022

original-brownbear reviewed Jan 24, 2022

View reviewed changes

joegallo added 4 commits January 24, 2022 14:08

This gets the tests passing again

56f4b9f

It's the first thing the reroute call itself actually called, so we're keeping a portion of a full reroute call here, but deferring the rest of it until the end of the executor loop.

Rename this variable, s/closeIndices/closedIndices/g

dc452c7

Merge branch 'master' into batch-settings-update

0badf30

Skip the reroute if the whole batch was a noop

be4de6c

joegallo requested a review from DaveCTurner January 24, 2022 19:59

joegallo marked this pull request as ready for review January 24, 2022 20:20

elasticmachine added the Team:Distributed Meta label for distributed team. label Jan 24, 2022

joegallo requested a review from original-brownbear January 25, 2022 13:47

original-brownbear reviewed Jan 25, 2022

View reviewed changes

original-brownbear approved these changes Jan 25, 2022

View reviewed changes

joegallo added 2 commits January 25, 2022 11:41

Rework test code, remove adaptAutoExpandReplicas hack

92bc39d

Merge branch 'master' into batch-settings-update

5528109

Update docs/changelog/82896.yaml

fd89e17

DaveCTurner approved these changes Jan 25, 2022

View reviewed changes

joegallo removed the v8.0.0 label Jan 25, 2022

joegallo merged commit 4bf8aec into elastic:master Jan 25, 2022

joegallo deleted the batch-settings-update branch January 25, 2022 21:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Batch Index Settings Update Requests#82896

Batch Index Settings Update Requests#82896
joegallo merged 13 commits intoelastic:masterfrom
joegallo:batch-settings-update

joegallo commented Jan 20, 2022 •

edited

Loading

Uh oh!

original-brownbear left a comment

Uh oh!

original-brownbear Jan 24, 2022

Uh oh!

joegallo Jan 24, 2022 •

edited

Loading

Uh oh!

elasticmachine commented Jan 24, 2022

Uh oh!

original-brownbear Jan 25, 2022

Uh oh!

DaveCTurner Jan 25, 2022

Uh oh!

joegallo Jan 25, 2022

Uh oh!

original-brownbear left a comment

Uh oh!

elasticsearchmachine commented Jan 25, 2022

Uh oh!

DaveCTurner left a comment

Uh oh!

joegallo commented Jan 25, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	private ClusterState executeClusterStateUpdateTask(ClusterState state, Runnable runnable) {
	ClusterState[] result = new ClusterState[1];
	doAnswer(invocationOnMock -> {
	ClusterStateUpdateTask task = (ClusterStateUpdateTask) invocationOnMock.getArguments()[1];
	result[0] = task.execute(state);
	return null;
	}).when(clusterService).submitStateUpdateTask(anyString(), any(ClusterStateUpdateTask.class), any());
	runnable.run();
	assertThat(result[0], notNullValue());
	return result[0];
	}

Conversation

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

Uh oh!

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

original-brownbear Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jan 24, 2022

Uh oh!

original-brownbear Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

DaveCTurner Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

joegallo Jan 25, 2022

Choose a reason for hiding this comment

Uh oh!

original-brownbear left a comment

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Jan 25, 2022

Uh oh!

DaveCTurner left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

joegallo commented Jan 20, 2022 •

edited

Loading

joegallo Jan 24, 2022 •

edited

Loading

joegallo commented Jan 25, 2022 •

edited

Loading