Introduce CCR stats endpoint #32350

jasontedor · 2018-07-25T05:01:18Z

This commit introduces the CCR stats endpoint which provides shard-level stats on the status of CCR follower tasks.

elasticmachine · 2018-07-25T05:01:19Z

Pinging @elastic/es-distributed

bleskes · 2018-07-25T12:20:33Z

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

+                        for (final Map.Entry<Integer, TransportCcrStatsAction.TaskResponse> shard : index.getValue().entrySet()) {
+                            builder.startObject(Integer.toString(shard.getKey()));
+                            {
+                                final ShardFollowNodeTask.Status status = shard.getValue().status();


why can't we use the status toXContent?

Because there are slight differences in what we present on the endpoint and the status x-content. For example: the endpoint doesn't present the index metadata version, and presents successful/failed fetches and bulk operations as total/successful.

re index metadata version - why not present everything we have? the purpose of status object is to report information about the running task, which I think is identical with the purpose of this api?

presents successful/failed fetches and bulk operations as total/successful.

We can change the status class to track these if you feel it's more natural. It's not a part of the task run time behavior so nothing stops us to align these.

re index metadata version - why not present everything we have?

This does not seem useful from an end-user perspective but we can add in the interest of simplification.

We can change the status class to track these if you feel it's more natural. It's not a part of the task run time behavior so nothing stops us to align these.

Tracking successful/failed feels more natural internally, presenting total/successful feels more natural but we can present successful/failed in the interest of simplification.

I pushed e3fc87a.

bleskes · 2018-07-25T12:22:29Z

Thanks @jasontedor . I asked a quick question before doing a full review. I'm looking at this under the following assumptions:

We will review what we really expose in follow up discussions and potentially change it. This PR is about the infra to expose stats.
I believe we need an index level summary (shards are an implementation detail). This is IMO also out of scope (and has some tricky aspects).

Let me know if you agree.

* ccr: Number of utilities for writing gradle integration tests (elastic#32282) Determine the minimum gradle version based on the wrapper (elastic#32226) Enable FIPS JVM in CI (elastic#32330) Build: Fix jarHell error I caused by last backport Use shadow plugin in ccr/qa Fix JarHell on X-Pack protocol

jasontedor · 2018-07-25T13:34:32Z

Your assumptions are correct.

bleskes

Thanks @jasontedor . I left some comments. I also miss a test for the TransportCcrStatsAction (the only thing I see that does that's not covered elsewhere is the matching of tasks) and a rest layer test.

bleskes · 2018-07-27T15:24:03Z

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

+                        for (final Map.Entry<Integer, TransportCcrStatsAction.TaskResponse> shard : index.getValue().entrySet()) {
+                            builder.startObject(Integer.toString(shard.getKey()));
+                            {
+                                final ShardFollowNodeTask.Status status = shard.getValue().status();


re index metadata version - why not present everything we have? the purpose of status object is to report information about the running task, which I think is identical with the purpose of this api?

presents successful/failed fetches and bulk operations as total/successful.

We can change the status class to track these if you feel it's more natural. It's not a part of the task run time behavior so nothing stops us to align these.

bleskes · 2018-07-27T15:29:50Z

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

+
+        private String indexName;
+
+        public void setIndexName(final String indexName) {


I think we should follow the same patterns as an indices stats request - i.e., we should except a list of indices or a wild card pattern.

I think this will require some effort. I will look into this tomorrow.

@bleskes I pushed 2ee14e3. I am not a fan of it since the TasksRequest#match method is now a lie so that we can defer index resolution to the transport action. This is a limitation of the tasks API that puts us here. I do not think we should block CCR on addressing this so we have this workaround for now. I dislike it but maybe it's tolerable for now to give us an API consistent with others. Let me know what you think.

bleskes · 2018-07-27T15:35:37Z

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

+    private final ReleasableLock readLock;
+    private final ReleasableLock writeLock;
+
+    {


I don't like the locking semantics - read/ write for stats updates and fully synced for class execution. I think we should stick with one. I understand you did it to make sure stats reading is consistent. My vote goes to the simplicity of synchronized access and making the status creation method synchronized as well. It's super light.

Sure, no problem. I pushed 9b5aa54 and 54f7d3c.

bleskes · 2018-07-27T15:40:55Z

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

+        listener.onResponse(new TaskResponse(task.getFollowShardId(), task.getStatus()));
+    }
+
+    public static class TaskResponse implements Writeable {


why is this not together with the other request/response object for this action? I'd rather have them together (here or in the action)

I pushed 42c309d.

bleskes · 2018-07-27T15:42:26Z

x-pack/plugin/ccr/src/test/java/org/elasticsearch/xpack/ccr/ShardChangesIT.java

@@ -490,7 +491,7 @@ public void testFollowIndex_lowMaxTranslogBytes() throws Exception {
                ShardFollowNodeTask.Status status = (ShardFollowNodeTask.Status) taskInfo.getStatus();
                assertThat(status, notNullValue());
                assertThat("incorrect global checkpoint " + shardFollowTaskParams,
-                    status.getFollowerGlobalCheckpoint(),
+                    status.followerGlobalCheckpoint(),


just wondering (I don't care much) - why did you move from the get* method naming?

For an indication there is no setter and the field is otherwise immutable.

* elastic/ccr: (57 commits) ShardFollowNodeTask should fetch operation once (elastic#32455) Do not expose hard-deleted docs in Lucene history (elastic#32333) Tests: Fix convert error tests to use fixed value (elastic#32415) IndicesClusterStateService should replace an init. replica with an init. primary with the same aId (elastic#32374) REST high-level client: parse back _ignored meta field (elastic#32362) [CI] Mute DocumentSubsetReaderTests testSearch Reject follow request if following setting not enabled on follower (elastic#32448) TEST: testDocStats should always use forceMerge (elastic#32450) TEST: avoid merge in testSegmentMemoryTrackedInBreaker TEST: Avoid deletion in FlushIT AwaitsFix IndexShardTests#testDocStats Painless: Add method type to method. (elastic#32441) Remove reference to non-existent store type (elastic#32418) [TEST] Mute failing FlushIT test Fix ordering of bootstrap checks in docs (elastic#32417) [TEST] Mute failing InternalEngineTests#testSeqNoAndCheckpoints Validate source of an index in LuceneChangesSnapshot (elastic#32288) [TEST] Mute failing testConvertLongHexError bump lucene version after backport Upgrade to Lucene-7.5.0-snapshot-608f0277b0 (elastic#32390) ...

bleskes

Thanks for the extra iterations @jasontedor . Code so far LGTM. I would still love to see a rest test but I presume this is just still in the works.

bleskes · 2018-08-02T06:20:19Z

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

+             * where we can decide on the basis of the cluster state whether or not the task matches any of the index patterns in the
+             * request.
+             */
+            return task instanceof ShardFollowNodeTask;


I believe this can be an assertion now? we filter for the task instance type in the transport action.

I would support throwing UnsupportedOperationException.

I pushed b9c5429.

bleskes · 2018-08-02T06:26:07Z

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

    private volatile long leaderGlobalCheckpoint;
-
+    private volatile long leaderMaxSeqNo;


I think these don't need to be volatile any more, now that we read under lock.

You're right. With a little extra synchronization, it is not needed on any of the variables here. I pushed 45aba55.

bleskes · 2018-08-02T06:26:38Z

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

-            e -> handleFailure(e, retryCounter, () -> sendShardChangesRequest(from, maxOperationCount, maxRequiredSeqNo, retryCounter)));
+                response -> {
+                    synchronized (ShardFollowNodeTask.this) {
+                        // noinspection NonAtomicOperationOnVolatileField


see comment about these not needing to be volatile, meaning we can get rid of this suppression.

I pushed 4fc75d5.

bleskes · 2018-08-02T06:30:09Z

x-pack/plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/rest/RestCcrStatsAction.java

+
+    public RestCcrStatsAction(final Settings settings, final RestController controller) {
+        super(settings);
+        controller.registerHandler(RestRequest.Method.GET, "/_xpack/ccr/_stats", this);


heads up - this will change (or can be changed now)

* ccr: (24 commits) Remove _xpack from CCR APIs (elastic#32563) TEST: Avoid merges in testRecoveryWithOutOfOrderDelete Logging: Make node name consistent in logger (elastic#31588) Mute SSLTrustRestrictionsTests on JDK 11 Increase max chunk size to 256Mb for repo-azure (elastic#32101) Docs: Fix README upgrade mention (elastic#32313) Changed ReindexRequest to use Writeable.Reader (elastic#32401) Mute KerberosAuthenticationIT Fix AutoIntervalDateHistogram.testReduce random failures (elastic#32301) fix no=>not typo (elastic#32463) Mute QueryProfilerIT#testProfileMatchesRegular() HLRC: Add delete watch action (elastic#32337) High-level client: fix clusterAlias parsing in SearchHit (elastic#32465) Fix calculation of orientation of polygons (elastic#27967) [Kerberos] Add missing javadocs (elastic#32469) [Kerberos] Remove Kerberos bootstrap checks (elastic#32451) Make get all app privs requires "*" permission (elastic#32460) Switch security to new style Requests (elastic#32290) Switch security spi example to new style Requests (elastic#32341) Painless: Add PainlessConstructor (elastic#32447) ...

jasontedor · 2018-08-03T02:05:02Z

@bleskes Would you take another look?

This commit introduces the CCR stats endpoint which provides shard-level stats on the status of CCR follower tasks.

Update the endpoint and the unit tests to reflect the changes brought in [1] [1] elastic/elasticsearch#32350

Update the endpoint and the unit tests to reflect the changes brought in [1]. Also simplify filtering ccr-stats related documents, similarly to the work done in [2]. [1] elastic/elasticsearch#32350 [2] 9b8974f Relates #546

Introduce CCR stats endpoint

1a34a2d

This commit introduces the CCR stats endpoint which provides shard-level stats on the status of CCR follower tasks.

jasontedor added >enhancement review :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features labels Jul 25, 2018

Remove extra blank line

194bf80

bleskes reviewed Jul 25, 2018

View reviewed changes

bleskes suggested changes Jul 27, 2018

View reviewed changes

jasontedor added 4 commits July 29, 2018 07:22

Synchronize with coordinators

9b5aa54

Remove unused imports

54f7d3c

Move TaskResponse

42c309d

Simplify stats

e3fc87a

jasontedor force-pushed the ccr-stats branch from 55300a8 to e3fc87a Compare July 29, 2018 20:21

jasontedor added 2 commits July 31, 2018 10:45

Allow index patterns

2ee14e3

bleskes suggested changes Aug 2, 2018

View reviewed changes

jasontedor added 7 commits August 2, 2018 20:58

Throw unsupported operation exception

b9c5429

Remove volatile

45aba55

Remove imports

58afd89

Remove suppressions

4fc75d5

Change endpoint

421d43f

Add REST test

250c53a

jasontedor added 3 commits August 3, 2018 06:54

Remove unused import

fefb041

Add comment

901cc54

Fix typo in comment

b0d2cf0

bleskes approved these changes Aug 3, 2018

View reviewed changes

jasontedor added 2 commits August 3, 2018 08:44

Fix formatting

496ee04

Use millis

9c32d93

jasontedor merged commit d640c9d into elastic:ccr Aug 3, 2018

jasontedor added a commit that referenced this pull request Aug 3, 2018

Introduce CCR stats endpoint (#32350)

83f10d7

This commit introduces the CCR stats endpoint which provides shard-level stats on the status of CCR follower tasks.

jasontedor deleted the ccr-stats branch August 3, 2018 13:30

jasontedor mentioned this pull request Aug 6, 2018

Introduce cross-cluster replication #30086

Closed

29 tasks

dliappis added a commit to dliappis/rally that referenced this pull request Aug 6, 2018

Update endpoint for CCR stats telemetry device

ab960e3

Update the endpoint and the unit tests to reflect the changes brought in [1] [1] elastic/elasticsearch#32350

dliappis added a commit to dliappis/rally that referenced this pull request Aug 6, 2018

Update endpoint for CCR stats telemetry device

9386485

Update the endpoint and the unit tests to reflect the changes brought in [1] [1] elastic/elasticsearch#32350

dliappis mentioned this pull request Aug 7, 2018

Update ccr stats endpoint elastic/rally#546

Merged

jasontedor mentioned this pull request Sep 30, 2018

Change CCR stats endpoint to be index-centric #34169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce CCR stats endpoint #32350

Introduce CCR stats endpoint #32350

jasontedor commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

bleskes Jul 25, 2018

jasontedor Jul 25, 2018 •

edited

Loading

bleskes Jul 27, 2018

jasontedor Jul 29, 2018 •

edited

Loading

bleskes commented Jul 25, 2018

jasontedor commented Jul 25, 2018

bleskes left a comment

bleskes Jul 27, 2018

bleskes Jul 27, 2018

jasontedor Jul 29, 2018

jasontedor Jul 31, 2018

bleskes Jul 27, 2018

jasontedor Jul 29, 2018 •

edited

Loading

bleskes Jul 27, 2018

jasontedor Jul 29, 2018

bleskes Jul 27, 2018

jasontedor Jul 29, 2018

bleskes left a comment

bleskes Aug 2, 2018

jasontedor Aug 3, 2018

jasontedor Aug 3, 2018

bleskes Aug 2, 2018

jasontedor Aug 3, 2018 •

edited

Loading

bleskes Aug 2, 2018

jasontedor Aug 3, 2018 •

edited

Loading

bleskes Aug 2, 2018

jasontedor commented Aug 3, 2018


		private String indexName;

		public void setIndexName(final String indexName) {

		private volatile long leaderGlobalCheckpoint;

		private volatile long leaderMaxSeqNo;

Introduce CCR stats endpoint #32350

Introduce CCR stats endpoint #32350

Conversation

jasontedor commented Jul 25, 2018

elasticmachine commented Jul 25, 2018

Choose a reason for hiding this comment

jasontedor Jul 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor Jul 29, 2018 • edited Loading

Choose a reason for hiding this comment

bleskes commented Jul 25, 2018

jasontedor commented Jul 25, 2018

bleskes 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

jasontedor Jul 29, 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

bleskes 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

jasontedor Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor Aug 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasontedor commented Aug 3, 2018

jasontedor Jul 25, 2018 •

edited

Loading

jasontedor Jul 29, 2018 •

edited

Loading

jasontedor Jul 29, 2018 •

edited

Loading

jasontedor Aug 3, 2018 •

edited

Loading

jasontedor Aug 3, 2018 •

edited

Loading