[CCR] Changed AutoFollowCoordinator to keep track of certain statistics by martijnvg · Pull Request #33684 · elastic/elasticsearch

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 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

                            }
                            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

+            clusterAliasResults.set(slot, result);
            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

Conversation

martijnvg commented Sep 13, 2018

Uh oh!

elasticmachine commented Sep 13, 2018

Uh oh!

martijnvg commented Sep 13, 2018

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn Sep 17, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnhatn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dnhatn Sep 17, 2018 •

edited

Loading