[CCR] Introduce leader index name & last fetch time stats to stats api response #33155

martijnvg · 2018-08-27T04:02:22Z

No description provided.

… api response.

elasticmachine · 2018-08-27T04:02:23Z

Pinging @elastic/es-distributed

jasontedor

I left a comment about using the relative time provider, so we would report how long it has been since the most recent fetch. In general, I think this changes like this would be better as two separate PRs, these are separate changes.

jasontedor · 2018-08-27T10:49:55Z

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

    private final Queue<Translog.Operation> buffer = new PriorityQueue<>(Comparator.comparing(Translog.Operation::seqNo));
    private final LinkedHashMap<Long, ElasticsearchException> fetchExceptions;

    ShardFollowNodeTask(long id, String type, String action, String description, TaskId parentTask, Map<String, String> headers,
-                        ShardFollowTask params, BiConsumer<TimeValue, Runnable> scheduler, final LongSupplier relativeTimeProvider) {
+                        ShardFollowTask params, BiConsumer<TimeValue, Runnable> scheduler, final LongSupplier relativeTimeProvider,
+                        LongSupplier currentTimeSupplier) {


I think we can use the relativeTimeSupplier and merely report the time (elapsed) since the last fetch.

martijnvg · 2018-08-27T11:28:29Z

...gin/ccr/src/test/java/org/elasticsearch/xpack/ccr/action/ShardFollowNodeTaskStatusTests.java

@@ -89,6 +89,7 @@ protected void assertEqualInstances(final ShardFollowNodeTask.Status expectedIns
                    anyOf(instanceOf(ElasticsearchException.class), instanceOf(IllegalStateException.class)));
            assertThat(entry.getValue().getCause().getMessage(), containsString(expected.getCause().getMessage()));
        }
+        assertThat(newInstance.timeSinceLastFetch(), equalTo(expectedInstance.timeSinceLastFetch()));


🤦‍♂️

martijnvg · 2018-08-27T11:30:58Z

@jasontedor I've updated the PR to use report time since last fetch instead of the timestamp the last fetch occurred. Next time I will split a change like this into 2 PRs.

because that would be confusing for the consumer of the stats api.

jasontedor

Overall looking good, I left a comment.

jasontedor · 2018-08-27T20:30:32Z

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

+             timeSinceLastFetchMillis = TimeUnit.NANOSECONDS.toMillis(relativeTimeProvider.getAsLong() - lastFetchTime);
+        } else {
+            // To avoid confusion when ccr didn't yet execute a fetch:
+            timeSinceLastFetchMillis = 0;


I don't think it should be 0, I think it should be -1 so that we can distinguish a fetch just happened less than 1ms ago from a fetch just happened.

This means a serialization change too, to support -1.

martijnvg · 2018-08-28T01:59:44Z

@jasontedor I've addressed that comment.

jasontedor

I left one important comment, the rest is cosmetic. We will get this in on the next iteration.

jasontedor · 2018-08-28T11:01:04Z

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

@@ -412,7 +416,21 @@ public ShardId getFollowShardId() {

    @Override
    public synchronized Status getStatus() {
+        final String leaderIndex;
+        if (params.getLeaderClusterAlias() != null) {
+            leaderIndex = params.getLeaderClusterAlias() + ":" + params.getLeaderShardId().getIndexName();


I think that we should avoid re-computing this value every time we get status (think of a monitoring system polling the stats every second). We are creating unnecessary garbage on every poll for every shard.

Additionally, that's a calculation done under a lock, a lock that synchronizes a lot of other methods in this class.

jasontedor · 2018-08-28T11:04:57Z

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

@@ -648,7 +679,14 @@ public long numberOfOperationsIndexed() {
            return fetchExceptions;
        }

+        private final long timeSinceLastFetchMillis;


Thank you for keeping these in the same order as the constructor parameters, etc. 🙏

jasontedor · 2018-08-28T11:05:05Z

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

        Status(
+                String leaderIndex,


This can be final.

jasontedor · 2018-08-28T11:05:23Z

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

@@ -648,7 +679,14 @@ public long numberOfOperationsIndexed() {
            return fetchExceptions;
        }

+        private final long timeSinceLastFetchMillis;
+
+        public long timeSinceLastFetch() {


Add Millis to the name.

martijnvg · 2018-08-28T11:56:19Z

Thanks @jasontedor. I've updated the PR.

jasontedor

LGTM.

* ccr: Fix ShardFollowNodeTask.Status equals and hash code (elastic#33189) Make soft-deletes settings final (elastic#33172) Only fetch mapping updates when necessary (elastic#33182)

…i response (#33155)

[CCR] Introduced leader index name and last fetch time stats to stats…

d7c5059

… api response.

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

martijnvg requested a review from jasontedor August 27, 2018 04:02

fixed rest test

72feafd

jasontedor requested changes Aug 27, 2018

View reviewed changes

iter

a4fd3f2

martijnvg commented Aug 27, 2018

View reviewed changes

martijnvg added 2 commits August 27, 2018 18:34

also verify leader index...

2d00c45

if lastFetchTime == 0 then do not compute elapsed time,

e8b430a

because that would be confusing for the consumer of the stats api.

jasontedor reviewed Aug 27, 2018

View reviewed changes

use -1 as initial value

2ad7eb4

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

2ac8a29

jasontedor requested changes Aug 28, 2018

View reviewed changes

iter

b49f1ef

jasontedor approved these changes Aug 28, 2018

View reviewed changes

Merge branch 'ccr' into ccr_expand_stats_api_response

3a00ed8

* ccr: Fix ShardFollowNodeTask.Status equals and hash code (elastic#33189) Make soft-deletes settings final (elastic#33172) Only fetch mapping updates when necessary (elastic#33182)

martijnvg merged commit 41c7fc8 into elastic:ccr Aug 29, 2018

martijnvg added a commit that referenced this pull request Aug 29, 2018

[CCR] Introduce leader index name & last fetch time stats to stats ap…

648c149

…i response (#33155)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR] Introduce leader index name & last fetch time stats to stats api response #33155

[CCR] Introduce leader index name & last fetch time stats to stats api response #33155

martijnvg commented Aug 27, 2018

elasticmachine commented Aug 27, 2018

jasontedor left a comment

jasontedor Aug 27, 2018

martijnvg Aug 27, 2018

martijnvg commented Aug 27, 2018

jasontedor left a comment

jasontedor Aug 27, 2018

jasontedor Aug 27, 2018

martijnvg commented Aug 28, 2018

jasontedor left a comment

jasontedor Aug 28, 2018

jasontedor Aug 28, 2018

jasontedor Aug 28, 2018

jasontedor Aug 28, 2018

jasontedor Aug 28, 2018

martijnvg commented Aug 28, 2018

jasontedor left a comment

[CCR] Introduce leader index name & last fetch time stats to stats api response #33155

[CCR] Introduce leader index name & last fetch time stats to stats api response #33155

Conversation

martijnvg commented Aug 27, 2018

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

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

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

martijnvg commented Aug 28, 2018

jasontedor left a comment

Choose a reason for hiding this comment