[CCR] Added auto follow patterns feature #33118

martijnvg · 2018-08-24T07:34:54Z

Auto Following Patterns is a cross cluster replication feature that
keeps track whether in the leader cluster indices are being created with
names that match with a specific pattern and if so automatically let
the follower cluster follow these newly created indices.

This change adds an AutoFollowCoordinator component that is only active
on the elected master node. Periodically this component checks the
the cluster state of remote clusters if there new leader indices that
match with configured auto follow patterns that have been defined in
AutoFollowMetadata custom metadata.

This change also adds two new APIs to manage auto follow patterns. A put
auto follow pattern api:

PUT /_ccr/_autofollow/{{remote_cluster}}
{
   "leader_index_pattern": ["logs-*", ...],
   "follow_index_pattern": "{{leader_index}}-copy",
   "max_concurrent_read_batches": 2
   ... // other optional parameters
}

and delete auto follow pattern api:

DELETE /_ccr/_autofollow/{{remote_cluster_alias}}

The auto follow patterns are directly tied to the remote cluster aliases
configured in the follow cluster.

Note that this PR is not feature complete and several things will be added
via follow up PRs. (auto follow patterns work correctly with security, component
that cleans up leader index UUIDS from auto follow metadata if leader index have
been removed, etc. see meta issue for more)

Co-authored-by: Jason Tedor jason@tedor.me

Relates to #33007

Auto Following Patterns is a cross cluster replication feature that keeps track whether in the leader cluster indices are being created with names that match with a specific pattern and if so automatically let the follower cluster follow these newly created indices. This change adds an `AutoFollowCoordinator` component that is only active on the elected master node. Periodically this component checks the the cluster state of remote clusters if there new leader indices that match with configured auto follow patterns that have been defined in `AutoFollowMetadata` custom metadata. This change also adds two new APIs to manage auto follow patterns. A put auto follow pattern api: ``` PUT /_ccr/_autofollow/{{remote_cluster}} { "leader_index_pattern": ["logs-*", ...], "follow_index_pattern": "{{leader_index}}-copy", "max_concurrent_read_batches": 2 ... // other optional parameters } ``` and delete auto follow pattern api: ``` DELETE /_ccr/_autofollow/{{remote_cluster_alias}} ``` The auto follow patterns are directly tied to the remote cluster aliases configured in the follow cluster. Relates to elastic#33007

elasticmachine · 2018-08-24T07:35:08Z

Pinging @elastic/es-distributed

jasontedor

Heya @martijnvg this is a big pull request that will take me some time to go through. I’ve started buffering some comments but rather than dropping a giant review on you, I’ll submit them as I go. I’ll pick more of this up tomorrow morning.

jasontedor · 2018-08-26T19:17:33Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java

+/**
+ * Custom metadata that contains auto follow patterns and what leader indices an auto follow pattern has already followed.
+ */
+public class AutoFollowMetadata extends AbstractNamedDiffable<MetaData.Custom> implements MetaData.Custom {


This has to implement XPackPlugin$MetaDataCustom otherwise the feature-aware check fails:

> Task :x-pack:plugin:core:featureAwareCheck FAILED checking for custom violations class [org/elasticsearch/xpack/core/ccr/AutoFollowMetadata] implements [org/elasticsearch/cluster/metadata/MetaData$Custom but does not implement [org/elasticsearch/xpack/core/XPackPlugin$XPackMetaDataCustom] Exception in thread "main" java.lang.IllegalStateException: found custom in X-Pack not extending appropriate X-Pack mix-in at org.elasticsearch.xpack.test.feature_aware.FeatureAwareCheck.main(FeatureAwareCheck.java:58)

The reason for this check is to prevent us from adding new metadata customs to X-Pack that to do not specify they require X-Pack. See #31020 and #31081.

Thanks. I didn't notice this, because I only ran: ./gradlew x-pack:plugin:ccr:check prior to opening this PR and the PR build failed for a different reason (mixed cluster qa tests).

jasontedor · 2018-08-26T19:32:19Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java

+    private static final ParseField PATTERNS_FIELD = new ParseField("patterns");
+    private static final ParseField FOLLOWED_LEADER_INDICES_FIELD = new ParseField("followed_leader_indices");
+
+    private static final ConstructingObjectParser<AutoFollowMetadata, Void> PARSER = new ConstructingObjectParser<>("autofollow",


Nit: autofollow -> auto_follow.

jasontedor · 2018-08-26T19:33:13Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java

+    private static final ParseField PATTERNS_FIELD = new ParseField("patterns");
+    private static final ParseField FOLLOWED_LEADER_INDICES_FIELD = new ParseField("followed_leader_indices");
+
+    private static final ConstructingObjectParser<AutoFollowMetadata, Void> PARSER = new ConstructingObjectParser<>("autofollow",


Let's apply @SuppressWarnings("unchecked") to this field so we do not have to see those obnoxious unchecked cast warnings.

Similarly for autofollow elsewhere, and for other unchecked casts in this file.

jasontedor · 2018-08-27T03:06:49Z

x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/ccr/AutoFollowMetadata.java

+
+    @Override
+    public Version getMinimalSupportedVersion() {
+        return Version.CURRENT.minimumCompatibilityVersion();


This is not right, this should be 6.5.0.

jasontedor

I reviewed the coordinator today. Overall I think this is a great start and we are merely polishing around the edges. I will try to review another big chunk of code later tonight, but I want to not block you on making further progress on this change so I submit another piece of the review now. I will probably keep doing it in chunks like this. Let me know if you would prefer a different approach.

jasontedor · 2018-08-27T20:52:04Z

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

+        clusterService.addStateApplier(this);
+    }
+
+    void doAutoFollow() {


Can it be private?

jasontedor · 2018-08-27T20:54:28Z

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

+                errorHolder.get().addSuppressed(failure);
+            }
+
+            if (executedRequests.incrementAndGet() == autoFollowMetadata.getPatterns().size()) {


Can we use a CountDown since it gives us safety about never over counting down?

jasontedor · 2018-08-27T20:56:37Z

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

+
+        Consumer<Exception> handler = e -> {
+            if (e != null) {
+                LOGGER.error("Failure occurred during auto following indices", e);


I think this should be at the warn level.

jasontedor · 2018-08-27T20:59:33Z

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

+            for (Map.Entry<String, AutoFollowMetadata.AutoFollowPattern> entry : autoFollowMetadata.getPatterns().entrySet()) {
+                String clusterAlias = entry.getKey();
+                AutoFollowMetadata.AutoFollowPattern autoFollowPattern = entry.getValue();
+                Client remoteClient = clusterAlias.equals("_local_") ? client : client.getRemoteClusterClient(clusterAlias);


The name of this variable is misleading since it might not be a remote client.

jasontedor · 2018-08-27T21:01:10Z

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

+                String clusterAlias = entry.getKey();
+                AutoFollowMetadata.AutoFollowPattern autoFollowPattern = entry.getValue();
+                Client remoteClient = clusterAlias.equals("_local_") ? client : client.getRemoteClusterClient(clusterAlias);
+                List<String> followedIndices = autoFollowMetadata.getFollowedLeaderIndexUUIDS().get(clusterAlias);


Lowercase s in UUIDS.

jasontedor · 2018-08-27T23:56:29Z

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

+                            }
+                        });
+                    };
+                    LOGGER.info("Auto following leader index [{}] as follow index [{}]", leaderIndexName, followIndexName);


I think the placement of this log message is misleading since we can still fail after this.

jasontedor · 2018-08-27T23:58:00Z

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

+
+            void createAndFollowApiCall(FollowIndexAction.Request followRequest, Consumer<Exception> handler) {
+                client.execute(CreateAndFollowIndexAction.INSTANCE, new CreateAndFollowIndexAction.Request(followRequest),
+                    ActionListener.wrap(r -> handler.accept(null), handler::accept));


This can be handler instead of handler::accept.

jasontedor · 2018-08-28T00:01:59Z

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

+                            autoFollowPattern.getIdleShardRetryDelay());
+
+                    // This runs on the elected master node, so we can update cluster state here:
+                    Consumer<Exception> handler = followError -> {


This is difficult to read because it breaks the usual linear flow of reading code. So we are defining a callback, and then within that defining another callback and then it's really the updateAutoMetadata that executes first, and then there's another callback defined. I think it might be easier if we broke it down into onSuccess/onFailure instead of the Consumer<Exception> so that we can see the logic separately and think of it as "this is what happens if there is success, and this is what happens if there is failure".

Because really what we are after here is:

try to create the follower index and follow the leader

if that succeeds, update the auto-follow metadata, then do some accounting

if that fails, do some accounting and track the failure

it would be nice if the code read a little closer to that.

jasontedor · 2018-08-28T00:04:09Z

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

+                                .build());
+                            return newState.build();
+                        };
+                        updateAutoMetadata(clusterStateUpdateFunction, updateError -> {


Maybe updateAutoFollowMetadata?

jasontedor · 2018-08-28T00:04:20Z

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

+                        });
+                    };
+                    LOGGER.info("Auto following leader index [{}] as follow index [{}]", leaderIndexName, followIndexName);
+                    createAndFollowApiCall(followRequest, handler);


I think that createAndFollow would be a fine name.

determine the follow index name into testable methods.

martijnvg · 2018-08-28T05:19:03Z

@jasontedor Thanks for the review. I've updated the PR.

I will probably keep doing it in chunks like this. Let me know if you would prefer a different approach.

I think that for this change this is the best way to get it reviewed.

to make sure that al least one shard is available to avoid 'all shards failed' error in search api. so instead of checking if index exists in cluster state a wait for yellow check is done.

jasontedor

Thanks @martijnvg, let's keep going on working on the coordinator. I left a few more high-level comments. Let's come up with a plan for these.

I am going to be submitting a review for the other components later tonight, most of it is fine, I don't see anything glaring.

This will free us to focus the remainder of our effort on polishing the coordinator.

jasontedor · 2018-08-28T21:57:44Z

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

+        };
+        AutoFollower operation = new AutoFollower(client, handler, autoFollowMetadata) {
+
+            void getLeaderClusterState(Client leaderClient, BiConsumer<ClusterState, Exception> handler) {


I can haz @Override annotations? 😄

whoops, not sure what happened to these annotations :) I'll add them back.

jasontedor · 2018-08-28T22:02:21Z

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

+            if (leaderIndicesToFollow.isEmpty()) {
+                finalise(null);
+            } else {
+                CountDown countDown = new CountDown(leaderIndicesToFollow.size());


Reusing the name countDown scares me here because it shadows the countDown from the AutoFollower. It is especially confusing with this block of code:

if (countDown.countDown()) { finalise(updateError); }

where that is counting down the shadower (the local countDown defined in handleClusterAlias and then finalise itself counts down another CountDown named countDown (the AutoFollower CountDown). Can we give these clearer names, and definitely not shadow?

Agreed. I'll rename them.

jasontedor · 2018-08-28T22:17:50Z

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

+                                LOGGER.debug("Successfully marked leader index [{}] as auto followed", leaderIndexName);
+                            }
+                            if (countDown.countDown()) {
+                                finalise(updateError);


So I think that something like this means that we are losing errors. So we have:

a pattern metricbeat-*

let's say that matches two indices in the leader cluster metricbeat-2018-08-27 and metricbeat-2018-08-28

we have an outer CountDown AutoFollower#countDown initialized to one for the single pattern

we have an inner CountDown countDown in handleClusterAlias initialized to two for the two indices

the first cluster state update task for the first index fails

we count down handleClusterAlias#countDown and it decrements to one; we do nothing

the second cluster state update task for the second index succeeds, or fails; either way, we would count down handleClusterAlias#countDown again and the exception from the first failure would be lost

Good point. I'll keep track of the errorHolder and suppress subsequent errors to the first error if multiple exceptions occur.

jasontedor · 2018-08-28T22:32:18Z

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

+                        // The coordinator always runs on the elected master node, so we can update cluster state here:
+                        updateAutoFollowMetadata(function, updateError -> {
+                            if (updateError != null) {
+                                LOGGER.error("Failed to mark leader index [" + leaderIndexName + "] as auto followed", updateError);


So I am concerned about the error handling here. What happens if we have created the follower and actively started replication, but failed to add the index to the metadata? I think that it means we will retry on the next auto follower execution, which will then fail because the follower index already exists. And we will keep retrying. I think we need some protection here.

I think that it means we will retry on the next auto follower execution, which will then fail because the follower index already exists. And we will keep retrying.

Agreed. that would be a bad situation. I suspect such a thing is possible when the current master node fails.

The only think that I can think of that we can do here without making this code more complicated if to have another background check that verifies that there is an entry for all leader indices being followed. We could check persistent tasks to figure this out or use the leader index metadata that we're going to add as custom to IndexMetaData. What do you think about this?

Yeah, I am thinking something along those lines too. Like: if a leader index matches a pattern, and we are already following it, then we can skip create and follow and only need to get that index into the metadata. Your approach of using the follower index metadata about the leader index it is following sounds like a good approach to me.

I think it then makes sense to do this in a follow up PR? (because the follower index metadata needs to be added first)

Yeah, I think so. I think we can put some skeleton code into place now, with a bunch of TODOs. Would you add this to the meta-issue?

I added an entry to #33007

…ture we can check for indices that are already being followed but we failed to update AutoMetaData#followedLeaderIndexUUIDs

martijnvg · 2018-08-29T03:53:59Z

@jasontedor I've updated the PR.

jasontedor

I left some comments about put auto-follow patterns, I will be following up shortly with comments about delete auto-follow patterns.

jasontedor · 2018-08-29T18:45:13Z

x-pack/plugin/src/test/resources/rest-api-spec/api/ccr.put_auto_follow_pattern.json

+    "documentation": "http://www.elastic.co/guide/en/elasticsearch/reference/current",
+    "methods": [ "PUT" ],
+    "url": {
+      "path": "/_ccr/_autofollow/{remote_cluster_alias}",


Let us make the endpoints /_ccr/auto_follow for both delete and put.

jasontedor · 2018-08-29T18:45:43Z

...lugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/rest/RestPutAutoFollowPatternAction.java

+
+    @Override
+    public String getName() {
+        return "ccr_put_auto_folow_pattern_action";


folow -> follow

jasontedor · 2018-08-29T18:46:21Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+        clusterStateRequest.metaData(true);
+
+        remoteClient.admin().cluster().state(clusterStateRequest, ActionListener.wrap(resp -> {
+            clusterService.submitStateUpdateTask("put_auto_follow_pattern-" + request.getRemoteClusterAlias(),


We can use dashes throughout: put-auto-follow-pattern-

jasontedor · 2018-08-29T18:46:44Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+
+        AutoFollowMetadata.AutoFollowPattern previousPattern = configurations.get(request.getRemoteClusterAlias());
+        List<String> followedIndexUUIDS = followedLeaderIndices.get(request.getRemoteClusterAlias());
+        if (followedIndexUUIDS == null) {


Lowercase s in UUIDs.

jasontedor · 2018-08-29T18:48:13Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+
+        AutoFollowMetadata currentAutoFollowMetadata = localState.metaData().custom(AutoFollowMetadata.TYPE);
+        Map<String, List<String>> followedLeaderIndices;
+        Map<String, AutoFollowMetadata.AutoFollowPattern> configurations;


Call it patterns?

jasontedor · 2018-08-29T18:49:33Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+    protected void masterOperation(PutAutoFollowPatternAction.Request request,
+                                   ClusterState state,
+                                   ActionListener<AcknowledgedResponse> listener) throws Exception {
+        final Client remoteClient;


Let us call this leaderClient.

jasontedor · 2018-08-29T19:11:39Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+            for (String newPattern : request.getLeaderIndexPatterns()) {
+                if (previousPattern.getLeaderIndexPatterns().contains(newPattern) == false) {
+                    for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) {
+                        if (Regex.simpleMatch(newPattern, indexMetaData.getIndex().getName())) {


We are doing matching like this in a few places; can we get a utility class AutoFollowPatterns now that has a match method that simply delegates to Regex#simpleMatch. I want this now so we don't have to hunt through the code later if we ever make the matching more intricate.

jasontedor · 2018-08-29T19:12:07Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+        } else {
+            for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) {
+                String[] patterns = request.getLeaderIndexPatterns().toArray(new String[0]);
+                if (Regex.simpleMatch(patterns, indexMetaData.getIndex().getName())) {


Delegate to AutoFollowPatterns#match.

This now looks identical to the markExistingIndicesAsAutoFollowed that I suggested above so I think we can collapse these into a single implementation parameterized by patterns. We would have:

diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java index bff2809f321..f94be2a3b41 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java @@ -111,22 +111,11 @@ public class TransportPutAutoFollowPatternAction extends // Mark existing leader indices as already auto followed: if (previousPattern != null) { - for (String newPattern : request.getLeaderIndexPatterns()) { - if (previousPattern.getLeaderIndexPatterns().contains(newPattern) == false) { - for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) { - if (Regex.simpleMatch(newPattern, indexMetaData.getIndex().getName())) { - followedIndexUUIDS.add(indexMetaData.getIndexUUID()); - } - } - } - } + markExistingIndicesMatchingNewPatternsAsAutoFollowed( + previousPattern, request.getLeaderIndexPatterns(), remoteClusterState.metaData(), followedIndexUUIDS); } else { - for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) { - String[] patterns = request.getLeaderIndexPatterns().toArray(new String[0]); - if (Regex.simpleMatch(patterns, indexMetaData.getIndex().getName())) { - followedIndexUUIDS.add(indexMetaData.getIndexUUID()); - } - } + final String[] patterns = request.getLeaderIndexPatterns().toArray(new String[0]); + markExistingIndicesMatchingPatternsAsAutoFollowed(patterns, remoteClusterState.metaData(), followedIndexUUIDS); } AutoFollowMetadata.AutoFollowPattern autoFollowPattern = new AutoFollowMetadata.AutoFollowPattern(request.getLeaderIndexPatterns(), @@ -141,6 +130,27 @@ public class TransportPutAutoFollowPatternAction extends return newState.build(); } + private static void markExistingIndicesMatchingNewPatternsAsAutoFollowed( + AutoFollowMetadata.AutoFollowPattern previousPattern, + List<String> leaderIndexPatterns, + MetaData leaderMetaData, + List<String> followedIndexUUIDS) { + final String[] newPatterns = leaderIndexPatterns + .stream() + .filter(p -> previousPattern.getLeaderIndexPatterns().contains(p) == false) + .toArray(String[]::new); + markExistingIndicesMatchingPatternsAsAutoFollowed(newPatterns, leaderMetaData, followedIndexUUIDS); + } + + private static void markExistingIndicesMatchingPatternsAsAutoFollowed( + String[] patterns, MetaData leaderMetaData, List<String> followedIndexUUIDS) { + for (final IndexMetaData indexMetaData : leaderMetaData) { + if (Regex.simpleMatch(patterns, indexMetaData.getIndex().getName())) { + followedIndexUUIDS.add(indexMetaData.getIndexUUID()); + } + } + } + @Override protected ClusterBlockException checkBlock(PutAutoFollowPatternAction.Request request, ClusterState state) { return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);

jasontedor · 2018-08-29T19:15:12Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+        if (previousPattern != null) {
+            for (String newPattern : request.getLeaderIndexPatterns()) {
+                if (previousPattern.getLeaderIndexPatterns().contains(newPattern) == false) {
+                    for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) {


Let us collapse some of the nesting here. For example, we can replace this loop with addIndicesMatchingPattern(newPattern, leaderClusterState.getMetaData()) whose implementation is exactly this loop. This method becomes unit testable too.

jasontedor · 2018-08-29T19:16:49Z

...cr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java

+
+        // Mark existing leader indices as already auto followed:
+        if (previousPattern != null) {
+            for (String newPattern : request.getLeaderIndexPatterns()) {


Collapse this whole loop into a method: markExistingIndicesAsFollowed.

Also I think this loop can be rewritten more simply. I would go through two transformations, like this:

diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java index bff2809f321..e3308c30345 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java @@ -111,15 +111,8 @@ public class TransportPutAutoFollowPatternAction extends // Mark existing leader indices as already auto followed: if (previousPattern != null) { - for (String newPattern : request.getLeaderIndexPatterns()) { - if (previousPattern.getLeaderIndexPatterns().contains(newPattern) == false) { - for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) { - if (Regex.simpleMatch(newPattern, indexMetaData.getIndex().getName())) { - followedIndexUUIDS.add(indexMetaData.getIndexUUID()); - } - } - } - } + markExistingIndicesAsAutoFollowed( + request.getLeaderIndexPatterns(), remoteClusterState.metaData(), previousPattern, followedIndexUUIDS); } else { for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) { String[] patterns = request.getLeaderIndexPatterns().toArray(new String[0]); @@ -141,6 +134,26 @@ public class TransportPutAutoFollowPatternAction extends return newState.build(); } + private static void markExistingIndicesAsAutoFollowed( + List<String> leaderIndexPatterns, + MetaData leaderMetaData, + AutoFollowMetadata.AutoFollowPattern previousPattern, + List<String> followedIndexUUIDS) { + for (String newPattern : leaderIndexPatterns) { + if (previousPattern.getLeaderIndexPatterns().contains(newPattern) == false) { + addIndicesMatchingPattern(leaderMetaData, followedIndexUUIDS, newPattern); + } + } + } + + private static void addIndicesMatchingPattern(MetaData leaderMetaData, List<String> followedIndexUUIDS, String newPattern) { + for (IndexMetaData indexMetaData : leaderMetaData) { + if (Regex.simpleMatch(newPattern, indexMetaData.getIndex().getName())) { + followedIndexUUIDS.add(indexMetaData.getIndexUUID()); + } + } + } + @Override protected ClusterBlockException checkBlock(PutAutoFollowPatternAction.Request request, ClusterState state) { return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);

And then a second iteration:

diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java index e3308c30345..73b4d44511f 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; public class TransportPutAutoFollowPatternAction extends TransportMasterNodeAction<PutAutoFollowPatternAction.Request, AcknowledgedResponse> { @@ -139,16 +140,12 @@ public class TransportPutAutoFollowPatternAction extends MetaData leaderMetaData, AutoFollowMetadata.AutoFollowPattern previousPattern, List<String> followedIndexUUIDS) { - for (String newPattern : leaderIndexPatterns) { - if (previousPattern.getLeaderIndexPatterns().contains(newPattern) == false) { - addIndicesMatchingPattern(leaderMetaData, followedIndexUUIDS, newPattern); - } - } - } - - private static void addIndicesMatchingPattern(MetaData leaderMetaData, List<String> followedIndexUUIDS, String newPattern) { + final String[] newPatterns = leaderIndexPatterns + .stream() + .filter(p -> previousPattern.getLeaderIndexPatterns().contains(p) == false) + .toArray(String[]::new); for (IndexMetaData indexMetaData : leaderMetaData) { - if (Regex.simpleMatch(newPattern, indexMetaData.getIndex().getName())) { + if (Regex.simpleMatch(newPatterns, indexMetaData.getIndex().getName())) { followedIndexUUIDS.add(indexMetaData.getIndexUUID()); } }

removes some of the nesting and lets us drop one of the methods that we just added. Then the final diff looks like:

diff --git a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java index bff2809f321..73b4d44511f 100644 --- a/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java +++ b/x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/TransportPutAutoFollowPatternAction.java @@ -30,6 +30,7 @@ import java.util.ArrayList; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.stream.Collectors; public class TransportPutAutoFollowPatternAction extends TransportMasterNodeAction<PutAutoFollowPatternAction.Request, AcknowledgedResponse> { @@ -111,15 +112,8 @@ public class TransportPutAutoFollowPatternAction extends // Mark existing leader indices as already auto followed: if (previousPattern != null) { - for (String newPattern : request.getLeaderIndexPatterns()) { - if (previousPattern.getLeaderIndexPatterns().contains(newPattern) == false) { - for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) { - if (Regex.simpleMatch(newPattern, indexMetaData.getIndex().getName())) { - followedIndexUUIDS.add(indexMetaData.getIndexUUID()); - } - } - } - } + markExistingIndicesAsAutoFollowed( + request.getLeaderIndexPatterns(), remoteClusterState.metaData(), previousPattern, followedIndexUUIDS); } else { for (IndexMetaData indexMetaData : remoteClusterState.getMetaData()) { String[] patterns = request.getLeaderIndexPatterns().toArray(new String[0]); @@ -141,6 +135,22 @@ public class TransportPutAutoFollowPatternAction extends return newState.build(); } + private static void markExistingIndicesAsAutoFollowed( + List<String> leaderIndexPatterns, + MetaData leaderMetaData, + AutoFollowMetadata.AutoFollowPattern previousPattern, + List<String> followedIndexUUIDS) { + final String[] newPatterns = leaderIndexPatterns + .stream() + .filter(p -> previousPattern.getLeaderIndexPatterns().contains(p) == false) + .toArray(String[]::new); + for (IndexMetaData indexMetaData : leaderMetaData) { + if (Regex.simpleMatch(newPatterns, indexMetaData.getIndex().getName())) { + followedIndexUUIDS.add(indexMetaData.getIndexUUID()); + } + } + } + @Override protected ClusterBlockException checkBlock(PutAutoFollowPatternAction.Request request, ClusterState state) { return state.blocks().globalBlockedException(ClusterBlockLevel.METADATA_WRITE);

jasontedor

I left a few more comments about put.

jasontedor · 2018-08-29T23:05:45Z

.../plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/PutAutoFollowPatternAction.java

+
+    public static class Request extends AcknowledgedRequest<Request> implements ToXContentObject {
+
+        static final ParseField REMOTE_CLUSTER_ALIAS_FIELD = new ParseField("remote_cluster_alias");


remote_cluster -> leader_cluster

jasontedor · 2018-08-29T23:06:03Z

.../plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/PutAutoFollowPatternAction.java

+            return request;
+        }
+
+        private String remoteClusterAlias;


leaderClusterAlias

jasontedor · 2018-08-29T23:06:14Z

.../plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/PutAutoFollowPatternAction.java

+        public ActionRequestValidationException validate() {
+            ActionRequestValidationException validationException = null;
+            if (remoteClusterAlias == null) {
+                validationException = addValidationError("remoteClusterAlias is missing", validationException);


leaderClusterAlias

jasontedor

I left some comments about delete auto follow patterns API.

jasontedor · 2018-08-29T23:07:44Z

...ugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/DeleteAutoFollowPatternAction.java

+
+    public static class Request extends AcknowledgedRequest<Request> {
+
+        private String remoteClusterAlias;


leaderClusterAlias

jasontedor · 2018-08-29T23:09:11Z

...ugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/action/DeleteAutoFollowPatternAction.java

+        public ActionRequestValidationException validate() {
+            ActionRequestValidationException validationException = null;
+            if (remoteClusterAlias == null) {
+                validationException = addValidationError("remoteClusterAlias is missing", validationException);


leaderClusterAlias

jasontedor · 2018-08-29T23:09:41Z

...in/ccr/src/main/java/org/elasticsearch/xpack/ccr/rest/RestDeleteAutoFollowPatternAction.java

+    @Override
+    protected RestChannelConsumer prepareRequest(RestRequest restRequest, NodeClient client) throws IOException {
+        Request request = new Request();
+        request.setRemoteClusterAlias(restRequest.param("remote_cluster_alias"));


leader_cluster_alias

jasontedor · 2018-08-29T23:10:37Z

...src/main/java/org/elasticsearch/xpack/ccr/action/TransportDeleteAutoFollowPatternAction.java

+    protected void masterOperation(DeleteAutoFollowPatternAction.Request request,
+                                   ClusterState state,
+                                   ActionListener<AcknowledgedResponse> listener) throws Exception {
+        clusterService.submitStateUpdateTask("put_auto_follow_pattern-" + request.getRemoteClusterAlias(),


put -> delete and _ -> - so: delete-auto-follow-pattern-

jasontedor · 2018-08-30T00:27:35Z

...src/main/java/org/elasticsearch/xpack/ccr/action/TransportDeleteAutoFollowPatternAction.java

+    static ClusterState innerDelete(DeleteAutoFollowPatternAction.Request request, ClusterState currentState) {
+        AutoFollowMetadata currentAutoFollowMetadata = currentState.metaData().custom(AutoFollowMetadata.TYPE);
+        if (currentAutoFollowMetadata == null) {
+            throw new ResourceNotFoundException("auto follow patterns for [{}] cluster alias are missing",


auto follow patterns for [{}] cluster alias are missing -> no auto-follow patterns for cluster alias [{}] found

jasontedor · 2018-08-30T02:12:10Z

...src/main/java/org/elasticsearch/xpack/ccr/action/TransportDeleteAutoFollowPatternAction.java

+            if (request.getRemoteClusterAlias().equals(configurationKey)) {
+                toRemove.add(configurationKey);
+            }
+        }


I am confused by this loop. Isn't configurations.get(request.getRemoteClusterAlias()) all we need here?

jasontedor · 2018-08-30T02:16:52Z

...src/main/java/org/elasticsearch/xpack/ccr/action/TransportDeleteAutoFollowPatternAction.java

+            }
+        }
+        if (toRemove.isEmpty()) {
+            throw new ResourceNotFoundException("auto follow patterns for [{}] cluster alias are missing",


I think that we should replace this with the same exception message that I suggested above.

🤦‍♂️ yup

jasontedor · 2018-08-30T02:19:19Z

...in/ccr/src/main/java/org/elasticsearch/xpack/ccr/rest/RestDeleteAutoFollowPatternAction.java

+
+    public RestDeleteAutoFollowPatternAction(Settings settings, RestController controller) {
+        super(settings);
+        controller.registerHandler(RestRequest.Method.DELETE, "/_ccr/_autofollow/{remote_cluster_alias}", this);


remote_cluster_alias -> leader_cluster_alias (and in the specification)

jasontedor · 2018-08-30T02:19:30Z

...in/ccr/src/main/java/org/elasticsearch/xpack/ccr/rest/RestDeleteAutoFollowPatternAction.java

+
+    public RestDeleteAutoFollowPatternAction(Settings settings, RestController controller) {
+        super(settings);
+        controller.registerHandler(RestRequest.Method.DELETE, "/_ccr/_autofollow/{remote_cluster_alias}", this);


_autofollow -> auto_follow

jasontedor · 2018-08-30T02:20:16Z

...src/main/java/org/elasticsearch/xpack/ccr/action/TransportDeleteAutoFollowPatternAction.java

+        final Map<String, AutoFollowMetadata.AutoFollowPattern> configurationsCopy = new HashMap<>(configurations);
+        final Map<String, List<String>> followedLeaderIndexUUIDSCopy =
+            new HashMap<>(currentAutoFollowMetadata.getFollowedLeaderIndexUUIDs());
+        for (String key : toRemove) {


There should be one or none, right? And if none, we can return the current cluster state.

🤦‍♂️ again

martijnvg · 2018-08-30T03:00:40Z

@jasontedor I've updated the PR.

* master: (64 commits) HLREST: add update by query API (elastic#32760) TEST: Increase timeout testFollowIndexAndCloseNode (elastic#33333) HLRC: ML Flush job (elastic#33187) HLRC: Adding ML Job stats (elastic#33183) LLREST: Drop deprecated methods (elastic#33223) Mute testSyncerOnClosingShard [DOCS] Moves machine learning APIs to docs folder (elastic#31118) Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test Adjust soft-deletes version after backport into 6.5 completely drop `index.shard.check_on_startup: fix` for 7.0 (elastic#33194) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi drop `index.shard.check_on_startup: fix` (elastic#32279) tracked at [DOCS] Moves ml folder from x-pack/docs to docs (elastic#33248) [DOCS] Move rollup APIs to docs (elastic#31450) [DOCS] Rename X-Pack Commands section (elastic#33005) TEST: Disable soft-deletes in ParentChildTestCase Fixes SecurityIntegTestCase so it always adds at least one alias (elastic#33296) ...

jasontedor

Nice work, LGTM. Let us get this in and start work on the follow-ups that we have discussed. We have smoothed this one out enough to iterate on.

martijnvg · 2018-09-06T05:44:41Z

server/src/main/java/org/elasticsearch/common/regex/Regex.java

-        }
-        return false;
+    public static boolean simpleMatch(final List<String> patterns, final String str) {
+        // #simpleMatch(String[], String) is likely to be inlined into this method


Auto Following Patterns is a cross cluster replication feature that keeps track whether in the leader cluster indices are being created with names that match with a specific pattern and if so automatically let the follower cluster follow these newly created indices. This change adds an `AutoFollowCoordinator` component that is only active on the elected master node. Periodically this component checks the the cluster state of remote clusters if there new leader indices that match with configured auto follow patterns that have been defined in `AutoFollowMetadata` custom metadata. This change also adds two new APIs to manage auto follow patterns. A put auto follow pattern api: ``` PUT /_ccr/_autofollow/{{remote_cluster}} { "leader_index_pattern": ["logs-*", ...], "follow_index_pattern": "{{leader_index}}-copy", "max_concurrent_read_batches": 2 ... // other optional parameters } ``` and delete auto follow pattern api: ``` DELETE /_ccr/_autofollow/{{remote_cluster_alias}} ``` The auto follow patterns are directly tied to the remote cluster aliases configured in the follow cluster. Relates to #33007 Co-authored-by: Jason Tedor <jason@tedor.me>

martijnvg requested a review from jasontedor August 24, 2018 07:34

martijnvg added review :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features labels Aug 24, 2018

martijnvg mentioned this pull request Aug 24, 2018

[CCR] Auto follow patterns #33007

Closed

10 tasks

jasontedor reviewed Aug 27, 2018

View reviewed changes

martijnvg added 4 commits August 27, 2018 10:38

Merge remote-tracking branch 'es/ccr' into ccr_auto_follow

e5160d5

iter

78c178a

single line for each field in hashcode method

80b517e

fixed delete auto follow pattern and added unit tests for it

f818a5d

jasontedor requested changes Aug 28, 2018

View reviewed changes

martijnvg added 3 commits August 28, 2018 09:10

Merge remote-tracking branch 'es/ccr' into ccr_auto_follow

1292150

iter and moved logic to determine leader indices to follow and logic to

c7c2287

determine the follow index name into testable methods.

make handleClusterAlias() method more readable

3baa210

since the test does not explicitly execute create_and_follow we need

812ebb7

to make sure that al least one shard is available to avoid 'all shards failed' error in search api. so instead of checking if index exists in cluster state a wait for yellow check is done.

jasontedor requested changes Aug 28, 2018

View reviewed changes

martijnvg added 6 commits August 29, 2018 10:04

Merge remote-tracking branch 'es/ccr' into ccr_auto_follow

1943f41

added missing @OverRide

5fcfb3e

rename

d2b0643

Keep track of all updateError instances

1634592

Added a TODO and passed down follower cluster state so that in the fu…

5edc457

…ture we can check for indices that are already being followed but we failed to update AutoMetaData#followedLeaderIndexUUIDs

pushed too fast

5045a62

jasontedor reviewed Aug 29, 2018

View reviewed changes

martijnvg added 3 commits August 30, 2018 08:29

Merge remote-tracking branch 'es/ccr' into ccr_auto_follow

df5fa78

changed api path from /_ccr/_autofollow to /_ccr/_auto_follow

91dedcd

iter

cf792d9

jasontedor reviewed Aug 30, 2018

View reviewed changes

martijnvg added 2 commits August 30, 2018 09:39

simplified innerPut() method and added AutoFollowPattern.match() method

4c4caee

iter delete autofollow api

01819ab

martijnvg added the >feature label Aug 30, 2018

jasontedor changed the base branch from ccr to master September 2, 2018 20:39

martijnvg and others added 4 commits September 5, 2018 20:43

Merge remote-tracking branch 'es/master' into ccr_auto_follow

720e53e

fixed yaml tests

cf984b3

Delegate to simpleMatch, it will probably be inlined anyway

f8e9ffe

Organize endpoints

7dea40c

jasontedor approved these changes Sep 6, 2018

View reviewed changes

martijnvg commented Sep 6, 2018

View reviewed changes

martijnvg merged commit a721d09 into elastic:master Sep 6, 2018

tomcallahan added v7.0.0 v6.5.0 labels Sep 8, 2018

Mpdreamz mentioned this pull request Dec 13, 2018

[meta] 6.5.0 Release elastic/elasticsearch-net#3457

Closed

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


		public static class Request extends AcknowledgedRequest<Request> implements ToXContentObject {

		static final ParseField REMOTE_CLUSTER_ALIAS_FIELD = new ParseField("remote_cluster_alias");


		public static class Request extends AcknowledgedRequest<Request> {

		private String remoteClusterAlias;

[CCR] Added auto follow patterns feature #33118

[CCR] Added auto follow patterns feature #33118

Conversation

martijnvg commented Aug 24, 2018 • edited Loading

elasticmachine commented Aug 24, 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Aug 28, 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

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 Aug 29, 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

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

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

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

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 Aug 30, 2018

jasontedor left a comment

Choose a reason for hiding this comment

martijnvg commented Aug 24, 2018 •

edited

Loading