Replicate index settings to followers by jasontedor · Pull Request #35089 · elastic/elasticsearch

jasontedor · 2018-10-30T15:37:09Z

This commit uses the index settings version so that a follower can replicate index settings changes as needed from the leader.

elasticmachine · 2018-10-30T15:37:11Z

Pinging @elastic/es-distributed

jasontedor · 2018-10-30T15:39:06Z

This code is really a POC, there is some work to be done to make this production-ready, and tests are needed. This effort will be picked up by @martijnvg.

martijnvg · 2018-11-01T10:35:45Z

@jasontedor I think I have made this change production ready.

The most significant change is that the PersistentTaskPlugin interface has changed. This was needed to pass down IndexScopedSettings to ShardFollowTasksExecutor in order to determine whether index settings are dynamic.

martijnvg · 2018-11-01T19:29:26Z

@elasticmachine run gradle build test

martijnvg · 2018-11-05T08:17:16Z

run package tests please

jasontedor

I left a few comments. 😉

jasontedor · 2018-11-06T13:18:38Z

server/src/main/java/org/elasticsearch/plugins/PersistentTaskPlugin.java

-                                                                        ThreadPool threadPool, Client client) {
+                                                                        ThreadPool threadPool,
+                                                                        Client client,
+                                                                        IndexScopedSettings indexScopedSettings) {


Pushing SettingsModule would feel better to me. IndexScopedSettings feels too specific exactly to CCR needs.

jasontedor · 2018-11-06T13:23:48Z

...ck/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowTasksExecutor.java

+                            }
+                        }
+
+                        if (onlyDynamicSettings) {


Maybe:

diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowTasksExecutor.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowTasksExecutor.java index 6cef9689409..2049bf39989 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowTasksExecutor.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowTasksExecutor.java @@ -170,16 +170,7 @@ public class ShardFollowTasksExecutor extends PersistentTasksExecutor<ShardFollo s -> existingSettings.get(s) == null || existingSettings.get(s).equals(settings.get(s)) == false ); - // Figure out whether the updated settings are all dynamic settings: - boolean onlyDynamicSettings = true; - for (String key : updatedSettings.keySet()) { - if (indexScopedSettings.isDynamicSetting(key) == false) { - onlyDynamicSettings = false; - break; - } - } - - if (onlyDynamicSettings) { + if (updatedSettings.keySet().stream().allMatch(indexScopedSettings::isDynamicSetting)) { // If only dynamic settings have been updated then just update these settings in follower index: final UpdateSettingsRequest updateSettingsRequest = new UpdateSettingsRequest(followIndex.getName()); updateSettingsRequest.settings(updatedSettings);

jasontedor · 2018-11-06T13:25:04Z

...ck/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowTasksExecutor.java

+                leaderClient.admin().cluster().state(clusterStateRequest, ActionListener.wrap(onResponse, errorHandler));
+            }
+
+            private void closeIndex(String followIndex, Settings updatedSettings, Runnable handler, Consumer<Exception> onFailure) {


This should probably be named closeIndexUpdateSettingsAndOpenIndex.

jasontedor · 2018-11-06T13:25:14Z

...ck/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/ShardFollowTasksExecutor.java

+                followerClient.admin().indices().close(closeRequest, ActionListener.wrap(onResponse, onFailure));
+            }
+
+            private void updateSettings(String followIndex, Settings updatedSettings, Runnable handler, Consumer<Exception> onFailure) {


updateSettingsAndOpenIndex

…SettingsModule` instead of `IndexScopedSettings`

martijnvg · 2018-11-06T14:03:27Z

@jasontedor I've addressed the comments.

jasontedor

I can't approve this PR. Would you also add a test for updating a setting that shouldn't percolate to the follower (so that the setting version indeed changes, but there's no settings change on the follower). Then I am good with this PR being merged.

Thank you for all the work here, and taking my POC to the finish line. This is great work by you (really, I gave you something rough, you polished it really nicely, and made it production-ready). 🙏

martijnvg · 2018-11-07T08:43:37Z

Thanks @jasontedor for reviewing!

Would you also add a test for updating a setting that shouldn't percolate to the follower (so that the setting version indeed changes, but there's no settings change on the follower)

I think the testUpdateWhiteListedLeaderIndexSettings() test is already testing this? It sets the index.number_of_replicas on leader index and verifies that that setting does not percolate to the follower index, but it does expect that the settings version is incremented on the follower index.

I'll rename this test the better reflect what it is testing.

jasontedor · 2018-11-07T16:41:49Z

Thanks for clarifying that test.

jasontedor

I can't approve this. LGTM.

This commit uses the index settings version so that a follower can replicate index settings changes as needed from the leader. Co-authored-by: Martijn van Groningen <martijn.v.groningen@gmail.com>

* master: (24 commits) Replicate index settings to followers (elastic#35089) Rename RealmConfig.globalSettings() to settings() (elastic#35330) [TEST] Cleanup FileUserPasswdStoreTests (elastic#35329) Scripting: Add back lookup vars in score script (elastic#34833) watcher: Fix integration tests to ensure correct start/stop of Watcher (elastic#35271) Remove ALL shard check in CheckShrinkReadyStep (elastic#35346) Use soft-deleted docs to resolve strategy for engine operation (elastic#35230) [ILM] Check shard and relocation status in AllocationRoutedStep (elastic#35316) Ignore date ranges containing 'now' when pre-processing a percolator query (elastic#35160) Add a frozen engine implementation (elastic#34357) Put a fake allocation id on allocate stale primary command (elastic#34140) [CCR] Enforce auto follow pattern name restrictions (elastic#35197) [ILM] rolling upgrade tests (elastic#35328) [ML] Add Missing data checking class (elastic#35310) Apply `ignore_throttled` also to concrete indices (elastic#35335) Make version field names more meaningful (elastic#35334) [CCR] Added HLRC support for pause follow API (elastic#35216) [Docs] Improve Convert Processor description (elastic#35280) [Painless] Removes extraneous compile method (elastic#35323) [CCR] Fail with a better error if leader index is red (elastic#35298) ...

* elastic/master: (25 commits) Fixes fast vector highlighter docs per issue 24318. (elastic#34190) [ML] Prevent notifications on deletion of a non existent job (elastic#35337) [CCR] Auto follow Coordinator fetch cluster state in system context (elastic#35120) Mute test for elastic#35361 Preserve `date_histogram` format when aggregating on unmapped fields (elastic#35254) Test: Mute failing SSL test Allow unmapped fields in composite aggregations (elastic#35331) [RCI] Add IndexShardOperationPermits.asyncBlockOperations(ActionListener<Releasable>) (elastic#34902) HLRC: reindex API with wait_for_completion false (elastic#35202) Add docs on JNA temp directory not being noexec (elastic#35355) [CCR] Adjust list of dynamic index settings that should be replicated (elastic#35195) Replicate index settings to followers (elastic#35089) Rename RealmConfig.globalSettings() to settings() (elastic#35330) [TEST] Cleanup FileUserPasswdStoreTests (elastic#35329) Scripting: Add back lookup vars in score script (elastic#34833) watcher: Fix integration tests to ensure correct start/stop of Watcher (elastic#35271) Remove ALL shard check in CheckShrinkReadyStep (elastic#35346) Use soft-deleted docs to resolve strategy for engine operation (elastic#35230) [ILM] Check shard and relocation status in AllocationRoutedStep (elastic#35316) Ignore date ranges containing 'now' when pre-processing a percolator query (elastic#35160) ...

This commit uses the index settings version so that a follower can replicate index settings changes as needed from the leader. Co-authored-by: Martijn van Groningen <martijn.v.groningen@gmail.com>

Replicate index settings to followers

836984b

This commit uses the index settings version so that a follower can replicate index settings changes as needed from the leader.

jasontedor added >non-issue WIP :Distributed/CCR Issues around the Cross Cluster State Replication features labels Oct 30, 2018

jasontedor assigned martijnvg Oct 30, 2018

jasontedor added the blocker label Oct 30, 2018

martijnvg added 9 commits October 31, 2018 10:52

Merge remote-tracking branch 'es/master' into ccr-index-settings-version

310bf87

Fixed tests

fc251f0

Added followerSettingsVersion to follow shard stats.

0ff9ff1

added unit tests

b71bee2

Added integration tests

05b51ad

Merge remote-tracking branch 'es/master' into ccr-index-settings-version

74f2914

Tidy up async calls

02f3635

added test for whitelisted settings

485aa47

Only close index if a setting is not dynamic.

91ca753

martijnvg removed the WIP label Nov 1, 2018

Fixed docs

acbfd12

Merge remote-tracking branch 'es/master' into ccr-index-settings-version

b33b07f

Merge remote-tracking branch 'es/master' into ccr-index-settings-version

89c5586

jasontedor commented Nov 6, 2018

View reviewed changes

martijnvg added 3 commits November 6, 2018 14:55

Merge remote-tracking branch 'es/master' into ccr-index-settings-version

d53360c

changed PersistentTaskPlugin#getPersistentTasksExecutor() to use …

e66bcae

…SettingsModule` instead of `IndexScopedSettings`

iter

e3a41bc

removed unused variable

64a9592

fixed checkstyle violation

1c2e8da

jasontedor commented Nov 7, 2018

View reviewed changes

Merge remote-tracking branch 'es/master' into ccr-index-settings-version

775d26d

renamed test

4eb9807

jasontedor commented Nov 7, 2018

View reviewed changes

Merge remote-tracking branch 'es/master' into ccr-index-settings-version

53f11bb

jasontedor merged commit 4f4fc3b into elastic:master Nov 8, 2018

jasontedor deleted the ccr-index-settings-version branch November 8, 2018 02:47

jasontedor removed the blocker label Nov 8, 2018

jasontedor mentioned this pull request Feb 1, 2019

CCR needs to revisit index settings when resuming following #34278

Closed

Conversation

jasontedor commented Oct 30, 2018

Uh oh!

elasticmachine commented Oct 30, 2018

Uh oh!

jasontedor commented Oct 30, 2018

Uh oh!

martijnvg commented Nov 1, 2018

Uh oh!

martijnvg commented Nov 1, 2018

Uh oh!

martijnvg commented Nov 5, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

jasontedor Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

jasontedor Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

jasontedor Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

jasontedor Nov 6, 2018

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Nov 6, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

martijnvg commented Nov 7, 2018

Uh oh!

jasontedor commented Nov 7, 2018

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants