CCR: replicates max seq_no of updates to follower #34051

dnhatn · 2018-09-25T14:10:34Z

This commit replicates the max_seq_no_of_updates on the leading index to
the primaries of the following index via ShardFollowNodeTask. The
max_seq_of_updates is then transmitted to the replicas of the follower
via replication requests (that's BulkShardOperationsRequest).

Relates #33656

This commit replicates the max_seq_no_of_updates on the leading index to the primaries of the following index via ShardFollowNodeTask. The max_seq_of_updates is then transmitted to the replicas of the follower via replication requests (that's BulkShardOperationsRequest). Relates elastic#33656

elasticmachine · 2018-09-25T14:10:36Z

Pinging @elastic/es-distributed

martijnvg

I took a look and it looks good. Left a question.

martijnvg · 2018-09-25T15:30:12Z

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

@@ -56,6 +57,7 @@

    private long leaderGlobalCheckpoint;
    private long leaderMaxSeqNo;
+    private volatile long leaderMaxSeqNoOfUpdatesOrDeletes = SequenceNumbers.UNASSIGNED_SEQ_NO;


If we provide leaderMaxSeqNoOfUpdatesOrDeletes at the line 298 then this field does not have to be volatile? Then in the case of retries we would not read an updated version, but I think that is ok?

You're correct. In the POC, I made that way but Boaz preferred capture it here. I am fine with either option.

I'm not sure exactly what @martijnvg meant, but I think this is has to be part of the state as we don't have direct connection between the read operations and the write operations. The read operations just populate the write buffer and the write operation read operations from the write buffer. Since the write buffer is class level state, I think it's easier to see correctness when you reason about an MSU field that relates to all ops in the write buffer. Note that I say easier because it maybe true without based on the fact that we always read the lowest sequence numbers from the buffer but IMO if that ends up working out it's still not worth the complexity.

@dnhatn does this need to be volatile? when do we read it out of lock?

@martijnvg concerned about the volatile. The current approach requires MSU to be volatile because we call sendBulkShardOperationsRequest without synchronization when handling write failures. We can make the MSU field without volatile by capturing it once after we populate the write buffer (with synchronization). I tend to prefer the approach without volatile because we won't change MSU when handling write failures. @bleskes WDYT?

I pushed adc5ae9

thanks @dnhatn!

bleskes

LGTM (with some comments for consideration).

bleskes · 2018-09-25T21:29:47Z

...rc/main/java/org/elasticsearch/xpack/ccr/action/bulk/TransportBulkShardOperationsAction.java

        return new CcrWritePrimaryResult(replicaRequest, location, primary, logger);
    }

    @Override
    protected WriteReplicaResult<BulkShardOperationsRequest> shardOperationOnReplica(
            final BulkShardOperationsRequest request, final IndexShard replica) throws Exception {
+        assert replica.getMaxSeqNoOfUpdatesOrDeletes() >= request.getMaxSeqNoOfUpdatesOrDeletes() :


bleskes · 2018-09-25T21:31:27Z

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

@@ -529,6 +543,53 @@ public void testAttemptToChangeCcrFollowingIndexSetting() throws Exception {
            "this setting is managed via a dedicated API"));
    }

+    public void testTransferMaxSeqNoOfUpdates() throws Exception {


@dnhatn what's the added value of this test compared to asserting at all other tests that the MSU on the follower is the same as the primary after operations?

Yep, I'll remove this test and reintroduce it when we have the optimization in the following engine.

bleskes · 2018-09-25T21:34:21Z

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

@@ -56,6 +57,7 @@

    private long leaderGlobalCheckpoint;
    private long leaderMaxSeqNo;
+    private volatile long leaderMaxSeqNoOfUpdatesOrDeletes = SequenceNumbers.UNASSIGNED_SEQ_NO;


I'm not sure exactly what @martijnvg meant, but I think this is has to be part of the state as we don't have direct connection between the read operations and the write operations. The read operations just populate the write buffer and the write operation read operations from the write buffer. Since the write buffer is class level state, I think it's easier to see correctness when you reason about an MSU field that relates to all ops in the write buffer. Note that I say easier because it maybe true without based on the fact that we always read the lowest sequence numbers from the buffer but IMO if that ends up working out it's still not worth the complexity.

@dnhatn does this need to be volatile? when do we read it out of lock?

dnhatn · 2018-09-25T22:25:33Z

@martijnvg I've addressed your question. Could you please have another look? Thank you.

martijnvg

LGTM

bleskes · 2018-09-26T10:14:59Z

Still LGTM

dnhatn · 2018-09-26T11:59:55Z

Thanks @martijnvg and @bleskes.

This commit replicates the max_seq_no_of_updates on the leading index to the primaries of the following index via ShardFollowNodeTask. The max_seq_of_updates is then transmitted to the replicas of the follower via replication requests (that's BulkShardOperationsRequest). Relates #33656

dnhatn added the :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features label Sep 25, 2018

dnhatn requested review from martijnvg, bleskes and jasontedor September 25, 2018 14:10

martijnvg reviewed Sep 25, 2018

View reviewed changes

bleskes approved these changes Sep 25, 2018

View reviewed changes

dnhatn added 2 commits September 25, 2018 18:16

pass max_seq_no_of_updates

adc5ae9

remove test

c82b3d0

martijnvg approved these changes Sep 26, 2018

View reviewed changes

dnhatn merged commit 48c169e into elastic:master Sep 26, 2018

dnhatn deleted the ccr-msu branch September 26, 2018 12:00

dnhatn added the backport pending label Sep 26, 2018

dnhatn removed the backport pending label Sep 27, 2018

tomcallahan added v7.0.0 v6.5.0 >enhancement labels Sep 28, 2018

dnhatn mentioned this pull request Sep 29, 2018

Uses auto generated timestamp with soft-deletes #33656

Closed

jasontedor removed v6.5.0 v7.0.0 labels Sep 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CCR: replicates max seq_no of updates to follower #34051

CCR: replicates max seq_no of updates to follower #34051

dnhatn commented Sep 25, 2018

elasticmachine commented Sep 25, 2018

martijnvg left a comment

martijnvg Sep 25, 2018

dnhatn Sep 25, 2018

bleskes Sep 25, 2018

dnhatn Sep 25, 2018

dnhatn Sep 25, 2018

martijnvg Sep 26, 2018

bleskes left a comment

bleskes Sep 25, 2018

bleskes Sep 25, 2018

dnhatn Sep 25, 2018

bleskes Sep 25, 2018

dnhatn commented Sep 25, 2018

martijnvg left a comment

bleskes commented Sep 26, 2018

dnhatn commented Sep 26, 2018

CCR: replicates max seq_no of updates to follower #34051

CCR: replicates max seq_no of updates to follower #34051

Conversation

dnhatn commented Sep 25, 2018

elasticmachine commented Sep 25, 2018

martijnvg 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

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

dnhatn commented Sep 25, 2018

martijnvg left a comment

Choose a reason for hiding this comment

bleskes commented Sep 26, 2018

dnhatn commented Sep 26, 2018