[CCR] Changed AutoFollowCoordinator to keep track of certain statistics #33684

martijnvg · 2018-09-13T18:10:45Z

The following stats are being kept track of:

The total number of times that auto following a leader index succeed.
The total number of times that auto following a leader index failed.
The total number of times that fetching a remote cluster state failed.
The most recent 256 auto follow failures per auto leader index
(e.g. create_and_follow api call fails) or cluster alias
(e.g. fetching remote cluster state fails).

Each auto follow run now produces a result that is being used to update
the stats being kept track of in AutoFollowCoordinator.

The transport and rest actions are added in a follow up PR.

Relates to #33007

The following stats are being kept track of: 1) The total number of times that auto following a leader index succeed. 2) The total number of times that auto following a leader index failed. 3) The total number of times that fetching a remote cluster state failed. 4) The most recent 256 auto follow failures per auto leader index (e.g. create_and_follow api call fails) or cluster alias (e.g. fetching remote cluster state fails). Each auto follow run now produces a result that is being used to update the stats being kept track of in AutoFollowCoordinator. Relates to elastic#33007

elasticmachine · 2018-09-13T18:10:47Z

Pinging @elastic/es-distributed

martijnvg · 2018-09-13T18:11:06Z

\cc @jasontedor

dnhatn

@martijnvg This looks good. I left some comments.

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

dnhatn · 2018-09-17T02:00:22Z

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

-                LOGGER.warn("failure occurred during auto-follower coordination", e);
+        Consumer<List<AutoFollowResult>> handler = results -> {
+            for (AutoFollowResult result : results) {
+                if (result.clusterStateFetchException != null) {


This flow is quite similar to updateStats. Should we combine these to a single method?

dnhatn · 2018-09-17T02:00:52Z

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

        private final ClusterState followerClusterState;
        private final AutoFollowMetadata autoFollowMetadata;

        private final CountDown autoFollowPatternsCountDown;
-        private final AtomicReference<Exception> autoFollowPatternsErrorHolder = new AtomicReference<>();
+        private final AtomicArray<AutoFollowResult> clusterAliasResults;


maybe just call it autoFollowResults?

dnhatn · 2018-09-17T02:03:19Z

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

-                for (Index indexToFollow : leaderIndicesToFollow) {
+                final AtomicArray<Tuple<Index, Exception>> results = new AtomicArray<>(leaderIndicesToFollow.size());
+                for (int i = 0; i < leaderIndicesToFollow.size(); i++) {
+                    final int slot = i;


I am wondering if we can use another data-structure (instead of AtomicArray) to avoid passing the slot-index around. It may be less error-prone because we now have two slot-indexes (clusterAliasSlot and slot) in handleClusterAlias method.

maybe it would be better if we just do not pass the clusterAliasSlot around. Let me see if this would be cleaner.

I think is better now: 90fb198 ?

@dnhatn The splitup of handleClusterAlias() method like we discussed privately: f501c10

dnhatn · 2018-09-17T02:03:48Z

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 (leaderIndicesCountDown.countDown()) {
-                                finalise(leaderIndicesErrorHolder.get());
+                                finalise(clusterAliasSlot, new AutoFollowResult(clusterAlias, results.asList()));


Can we assert that every slot is assigned?

dnhatn · 2018-09-17T02:04:02Z

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

            if (autoFollowPatternsCountDown.countDown()) {
-                handler.accept(autoFollowPatternsErrorHolder.get());
+                handler.accept(clusterAliasResults.asList());


Can we assert that every slot is assigned?

dnhatn · 2018-09-17T02:05:21Z

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

+
+        AutoFollowResult(String clusterAlias) {
+            this.clusterAlias = clusterAlias;
+            this.clusterStateFetchException = null;


Maybe just delegate to this(clusterAlias, null)?

dnhatn · 2018-09-17T02:06:49Z

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

+public class AutoFollowStats implements Writeable, ToXContentObject {
+
+    private static final ParseField NUMBER_OF_SUCCESSFUL_INDICES_AUTO_FOLLOWED =
+        new ParseField("number_of_successful_indices_auto_followed");


I am not sure if indices_auto_followed is a right term. @jasontedor WDYT?

maybe just number_of_successful_followed_indices?

Yes, I like this name. And I think we don't need ed here.

I pushed: 3a15fe6

…tor_stats

…method, this avoids handleClusterAlias() having to know about clusterAliasSlot parameter.

…tor_stats

dnhatn

LGTM. Thanks @martijnvg.

dnhatn · 2018-09-18T01:49:54Z

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

+                            getLeaderIndicesToFollow(autoFollowPattern, leaderClusterState, followerClusterState, followedIndices);
+                        if (leaderIndicesToFollow.isEmpty()) {
+                            finalise(slot, new AutoFollowResult(clusterAlias));
+                        }else {


nit: a space before else.

…cs (#33684) The following stats are being kept track of: 1) The total number of times that auto following a leader index succeed. 2) The total number of times that auto following a leader index failed. 3) The total number of times that fetching a remote cluster state failed. 4) The most recent 256 auto follow failures per auto leader index (e.g. create_and_follow api call fails) or cluster alias (e.g. fetching remote cluster state fails). Each auto follow run now produces a result that is being used to update the stats being kept track of in AutoFollowCoordinator. Relates to #33007

martijnvg added review :Distributed/CCR Issues around the Cross Cluster State Replication features labels Sep 13, 2018

martijnvg requested a review from dnhatn September 13, 2018 18:10

fixed checkstyle violation

c30d440

dnhatn reviewed Sep 17, 2018

View reviewed changes

martijnvg mentioned this pull request Sep 17, 2018

[CCR] Auto follow patterns #33007

Closed

10 tasks

martijnvg added 7 commits September 17, 2018 07:53

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

0804c37

…tor_stats

iter

59cde1a

let autoFollowIndices() handle passing down the result to finilise() …

90fb198

…method, this avoids handleClusterAlias() having to know about clusterAliasSlot parameter.

rename

3a15fe6

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

c602caf

…tor_stats

split up handleClusterAlias() method

f501c10

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

1bb6800

…tor_stats

dnhatn approved these changes Sep 18, 2018

View reviewed changes

added space

484e576

martijnvg merged commit 47b86d6 into elastic:master Sep 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR] Changed AutoFollowCoordinator to keep track of certain statistics #33684

[CCR] Changed AutoFollowCoordinator to keep track of certain statistics #33684

martijnvg commented Sep 13, 2018

elasticmachine commented Sep 13, 2018

martijnvg commented Sep 13, 2018

dnhatn left a comment

dnhatn Sep 17, 2018

dnhatn Sep 17, 2018 •

edited

Loading

dnhatn Sep 17, 2018

martijnvg Sep 17, 2018

martijnvg Sep 17, 2018

martijnvg Sep 17, 2018

dnhatn Sep 17, 2018

dnhatn Sep 17, 2018

dnhatn Sep 17, 2018

dnhatn Sep 17, 2018

martijnvg Sep 17, 2018

dnhatn Sep 17, 2018

martijnvg Sep 17, 2018

dnhatn left a comment

dnhatn Sep 18, 2018

[CCR] Changed AutoFollowCoordinator to keep track of certain statistics #33684

[CCR] Changed AutoFollowCoordinator to keep track of certain statistics #33684

Conversation

martijnvg commented Sep 13, 2018

elasticmachine commented Sep 13, 2018

martijnvg commented Sep 13, 2018

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn Sep 17, 2018 • edited Loading

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

dnhatn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn Sep 17, 2018 •

edited

Loading