[CCR] Clean followed leader index UUIDs in auto follow metadata #36408

martijnvg · 2018-12-09T18:34:02Z

The auto follow coordinator keeps track of the UUIDs of indices that it has followed. The index UUID strings need to be cleaned up in the case that these indices are removed in the remote cluster.

Relates to #33007

The auto follow coordinator keeps track of the UUIDs of indices that it has followed. The index UUID strings need to be cleaned up in the case that these indices are removed in the remote cluster. Relates to elastic#33007

elasticmachine · 2018-12-09T18:34:04Z

Pinging @elastic/es-distributed

jasontedor

Looks good. I left a few comments. They are pretty minor.

jasontedor · 2018-12-09T19:48:31Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

-                        }
-                        i++;
-                    }
+            getLeaderClusterState(remoteCluster, (remoteClusterState, remoteError) -> {


I guess this should be renamed to getRemoteClusterState now.

jasontedor · 2018-12-09T19:50:43Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

-                    for (String autoFollowPatternName : patterns) {
-                        results.add(new AutoFollowResult(autoFollowPatternName, e));
+                    for (int i = 0; i < patterns.size(); i++) {
+                        String autoFollowPatternName = patterns.get(i);


Maybe an assertion that remoteError is not null?

jasontedor · 2018-12-09T19:55:18Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

@@ -460,6 +467,56 @@ static String getFollowerIndexName(AutoFollowPattern autoFollowPattern, String l
            };
        }

+        void cleanFollowedLeaderIndices(final ClusterState remoteClusterState,
+                                                final List<String> patterns) {


These parameters should fit on online.

jasontedor · 2018-12-09T19:55:48Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

@@ -460,6 +467,56 @@ static String getFollowerIndexName(AutoFollowPattern autoFollowPattern, String l
            };
        }

+        void cleanFollowedLeaderIndices(final ClusterState remoteClusterState,


Continuing with the theme, let us leave "leader" out of this method name and instead prefer "remote".

jasontedor · 2018-12-09T19:55:57Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

+            });
+        }
+
+        static Function<ClusterState, ClusterState> cleanFollowedLeaderIndices(final MetaData remoteMetadata,


Leader -> Remote.

jasontedor · 2018-12-09T19:56:43Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

+        }
+
+        static Function<ClusterState, ClusterState> cleanFollowedLeaderIndices(final MetaData remoteMetadata,
+                                                                               final List<String> autoFollowPatternNames) {


How about

diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java index b3e437bc381..8fe89b3ae35 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java @@ -476,8 +476,8 @@ public class AutoFollowCoordinator implements ClusterStateListener { }); } - static Function<ClusterState, ClusterState> cleanFollowedLeaderIndices(final MetaData remoteMetadata, - final List<String> autoFollowPatternNames) { + static Function<ClusterState, ClusterState> cleanFollowedLeaderIndices( + final MetaData remoteMetadata, final List<String> autoFollowPatternNames) { return currentState -> { AutoFollowMetadata currentAutoFollowMetadata = currentState.metaData().custom(AutoFollowMetadata.TYPE); Map<String, List<String>> newFollowedIndexUUIDS = new HashMap<>(currentAutoFollowMetadata.getFollowedLeaderIndexUUIDs());

jasontedor · 2018-12-09T20:09:11Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

+                                                                               final List<String> autoFollowPatternNames) {
+            return currentState -> {
+                AutoFollowMetadata currentAutoFollowMetadata = currentState.metaData().custom(AutoFollowMetadata.TYPE);
+                Map<String, List<String>> newFollowedIndexUUIDS = new HashMap<>(currentAutoFollowMetadata.getFollowedLeaderIndexUUIDs());


I think this needs a rename. I got confused by it a few times later on in the code. Perhaps autoFollowPatternNameToFollowedIndexUUIDs?

jasontedor · 2018-12-09T20:10:16Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

+                        continue;
+                    }
+
+                    List<String> followedLeaderIndices = new ArrayList<>(newFollowedIndexUUIDS.get(autoFollowPatternName));


followedLeaderIndices -> followedIndexUUIDs?

…_followed_indices

…es in cluster, due that the fact that no good diffs could be determined of AutoFollowMetadata.

martijnvg · 2018-12-10T06:34:39Z

@jasontedor I've updated the PR.

I noticed that new test (AutoFollowIT#testCleanFollowedLeaderIndexUUIDs) was randomly failing. It turned out that because AutoFollowMetadata did not properly implement equals / hashcode and this is problematic, because AutoFollowMetadata extends AbstractNamedDiffable. In the test AutoFollowMetadata was introspected from an arbitrary node, but sometimes the followedLeaderIndexUUIDs did not contain the expected values. Changes to the headers and followedLeaderIndexUUIDs fields were never detected and therefor other nodes never got a cluster state with updates to these fields. The AutoFollowMetadataTests should have detected this problem.

…_followed_indices

jasontedor

The production code LGTM. Sorry for one more naming nit though. No need for another round!

jasontedor · 2018-12-11T15:30:27Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/AutoFollowCoordinator.java

+                if (leaderIndicesToFollow.isEmpty()) {
+                    finalise(slot, new AutoFollowResult(autoFollowPatternName));
+                } else {
+                    List<Tuple<String, AutoFollowPattern>> patternsForTheSameLeaderCluster = autoFollowMetadata.getPatterns()


LeaderCluster -> RemoteCluster?

I should have done find and replace case insensitive... it is more difficult to get rid of leaderCluster and variants than I thought it would be.

…_followed_indices

The auto follow coordinator keeps track of the UUIDs of indices that it has followed. The index UUID strings need to be cleaned up in the case that these indices are removed in the remote cluster. Relates to #33007

[CCR] Clean followed leader index UUIDs

c2978c0

The auto follow coordinator keeps track of the UUIDs of indices that it has followed. The index UUID strings need to be cleaned up in the case that these indices are removed in the remote cluster. Relates to elastic#33007

martijnvg added >enhancement v7.0.0 :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features v6.6.0 labels Dec 9, 2018

martijnvg requested a review from jasontedor December 9, 2018 18:34

jasontedor reviewed Dec 9, 2018

View reviewed changes

martijnvg added 4 commits December 10, 2018 06:40

Merge remote-tracking branch 'es/master' into ccr_auto_follower_clean…

716775e

…_followed_indices

iter

3ef46dd

a few more leader -> remote renames

3b595e6

yikes: AutoFollowMetadata was never correctly serialized to other nod…

0b7ac42

…es in cluster, due that the fact that no good diffs could be determined of AutoFollowMetadata.

Merge remote-tracking branch 'es/master' into ccr_auto_follower_clean…

895ca17

…_followed_indices

jasontedor approved these changes Dec 11, 2018

View reviewed changes

martijnvg added 3 commits December 11, 2018 19:44

Merge remote-tracking branch 'es/master' into ccr_auto_follower_clean…

9bcb54a

…_followed_indices

followedLeaderIndices -> followedIndexUUIDs

c542fe5

Merge remote-tracking branch 'es/master' into ccr_auto_follower_clean…

a380673

…_followed_indices

This was referenced Dec 12, 2018

[CCR] Change AutofollowCoordinator to use wait_for_metadata_version #36264

Merged

[CCR] Auto follow patterns #33007

Closed

martijnvg merged commit 4a825e2 into elastic:master Dec 12, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR] Clean followed leader index UUIDs in auto follow metadata #36408

[CCR] Clean followed leader index UUIDs in auto follow metadata #36408

martijnvg commented Dec 9, 2018

elasticmachine commented Dec 9, 2018

jasontedor left a comment

jasontedor Dec 9, 2018

jasontedor Dec 9, 2018

jasontedor Dec 9, 2018

jasontedor Dec 9, 2018

jasontedor Dec 9, 2018

jasontedor Dec 9, 2018

jasontedor Dec 9, 2018

jasontedor Dec 9, 2018

martijnvg commented Dec 10, 2018

jasontedor left a comment

jasontedor Dec 11, 2018

martijnvg Dec 11, 2018

[CCR] Clean followed leader index UUIDs in auto follow metadata #36408

[CCR] Clean followed leader index UUIDs in auto follow metadata #36408

Conversation

martijnvg commented Dec 9, 2018

elasticmachine commented Dec 9, 2018

jasontedor 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Dec 10, 2018

jasontedor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment