Introduce retention lease syncing #37398

jasontedor · 2019-01-14T04:18:28Z

This commit introduces retention lease syncing from the primary to its replicas when a new retention lease is added. A follow-up commit will add a background sync of the retention leases as well so that renewed retention leases are synced to replicas.

Relates #37165

This commit introduces retention lease syncing from the primary to its replicas when a new retention lease is added. A follow-up commit will add a background sync of the retention leases as well so that renewed retention leases are synced to replicas.

elasticmachine · 2019-01-14T04:18:30Z

Pinging @elastic/es-distributed

jasontedor · 2019-01-14T04:51:25Z

There will be a few follow-ups to this PR:

background sync of replication leases to replicas
replicas will not expire retention leases, only the primary will do that

jasontedor · 2019-01-14T05:26:02Z

@elasticmachine run gradle build tests 1

dnhatn

I left two comments but this looks really good.

server/src/main/java/org/elasticsearch/index/seqno/ReplicationTracker.java

dnhatn · 2019-01-14T06:25:45Z

server/src/main/java/org/elasticsearch/index/seqno/RetentionLeaseSyncAction.java

+    }
+
+    @Override
+    protected ReplicaResult shardOperationOnReplica(final Request request, final IndexShard replica) throws Exception {


I think we should handle BWC where old replicas don't have this action yet. Maybe add an upgrade test as a follow-up?

server/src/main/java/org/elasticsearch/index/seqno/RetentionLeaseSyncAction.java

server/src/test/java/org/elasticsearch/index/seqno/RetentionLeaseSyncIT.java

bleskes

I did a high level pass. Things looks good but I wanted to raise points for discussion.

server/src/main/java/org/elasticsearch/index/seqno/ReplicationTracker.java

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java

* master: (28 commits) Introduce retention lease serialization (elastic#37447) Update Delete Watch to allow unknown fields (elastic#37435) Make finalize step of recovery source non-blocking (elastic#37388) Update the default for include_type_name to false. (elastic#37285) Security: remove SSL settings fallback (elastic#36846) Adding mapping for hostname field (elastic#37288) Relax assertSameDocIdsOnShards assertion Reduce recovery time with compress or secure transport (elastic#36981) Implement ccr file restore (elastic#37130) Fix Eclipse specific compilation issue (elastic#37419) Performance fix. Reduce deprecation calls for the same bulk request (elastic#37415) [ML] Use String rep of Version in map for serialisation (elastic#37416) Cleanup Deadcode in Rest Tests (elastic#37418) Mute IndexShardRetentionLeaseTests.testCommit elastic#37420 unmuted test Remove unused index store in directory service Improve CloseWhileRelocatingShardsIT (elastic#37348) Fix ClusterBlock serialization and Close Index API logic after backport to 6.x (elastic#37360) Update the scroll example in the docs (elastic#37394) Update analysis.asciidoc (elastic#37404) ...

* master: Add simple method to write collection of writeables (elastic#37448) Fix retention lease commit test

* master: Reformat some classes in the index universe [DOCS] Add watcher context examples (elastic#36565)

* master: Add run under primary permit method (elastic#37440)

jasontedor · 2019-01-25T22:26:42Z

@dnhatn This is ready for another round. I will open follow-ups for:

syncing of retention leases on expiration
periodic syncing of retention leases
periodic flushing of retention leases

I am ready to open these in rapid succession. 😇

* elastic/master: (68 commits) Fix potential IllegalCapacityException in LLRC when selecting nodes (elastic#37821) Mute CcrRepositoryIT#testFollowerMappingIsUpdated Fix S3 Repository ITs When Docker is not Available (elastic#37878) Pass distribution type through to docs tests (elastic#37885) Mute SharedClusterSnapshotRestoreIT#testSnapshotCanceledOnRemovedShard SQL: Fix casting from date to numeric type to use millis (elastic#37869) Migrate o.e.i.r.RecoveryState to Writeable (elastic#37380) ML: removing unnecessary upgrade code (elastic#37879) Relax cluster metadata version check (elastic#37834) Mute TransformIntegrationTests#testSearchTransform Refactored GeoHashGrid unit tests (elastic#37832) Fixes for a few randomized agg tests that fail hasValue() checks Geo: replace intermediate geo objects with libs/geo (elastic#37721) Remove NOREPLACE for /etc/elasticsearch in rpm and deb (elastic#37839) Remove "reinstall" packaging tests (elastic#37851) Add unit tests for ShardStateAction's ShardStartedClusterStateTaskExecutor (elastic#37756) Exit batch files explictly using ERRORLEVEL (elastic#29583) TransportUnfollowAction should increase settings version (elastic#37859) AsyncTwoPhaseIndexerTests race condition fixed (elastic#37830) Do not allow negative variances (elastic#37384) ...

server/src/test/java/org/elasticsearch/index/seqno/RetentionLeaseSyncActionTests.java

…aseSyncActionTests.java

jasontedor · 2019-01-26T11:20:31Z

@elasticmachine run elasticsearch-ci/2
@elasticmachine run elasticsearch-ci/default-distro

jasontedor · 2019-01-26T12:48:40Z

@elasticmachine run elasticsearch-ci/2

dnhatn

I left a minor comment and two points to discuss. LGTM. Thanks @jasontedor.

server/src/main/java/org/elasticsearch/index/seqno/ReplicationTracker.java

dnhatn · 2019-01-26T18:59:00Z

server/src/main/java/org/elasticsearch/index/seqno/RetentionLeaseSyncAction.java

+        Objects.requireNonNull(replica);
+        replica.updateRetentionLeasesOnReplica(request.getRetentionLeases());
+        // we flush to ensure that retention leases are committed
+        flush(replica);


We may need to deal with the fact that a sequence-based or file-based recovery with a synced-flush does not propagate the retention leases to a recovering replica.

Yes, and also hand-off on relocation. I intend to address these later.

dnhatn · 2019-01-26T19:03:47Z

server/src/main/java/org/elasticsearch/index/seqno/RetentionLeaseSyncAction.java

+        Objects.requireNonNull(request);
+        Objects.requireNonNull(primary);
+        // we flush to ensure that retention leases are committed
+        flush(primary);


If the primary recovers from the safe commit, we will lose the committed retention leases. Should we copy them in Store#trimUnsafeCommits ?

Good call. Let us get that in a follow-up.

dnhatn · 2019-01-26T19:55:36Z

I am not sure, but maybe we should revisit Yannick's idea which stores the retention leases in the cluster state? Handling the retention leases for an intermediate cluster of a chain replication with a remote recovery might be tricky.

When adding a retention lease, we make a reference copy of the retention leases under lock and then make a copy of that collection outside of the lock. However, since we merely copied a reference to the retention leases, after leaving a lock the underlying collection could change on us. Rather, we want to copy these under lock. This commit adds a dedicated method for doing this, asserts that we hold a lock when we use this method, and changes adding a retention lease to use this method. This commit was intended to be included with #37398 but was pushed to the wrong branch.

This commit introduces retention lease syncing from the primary to its replicas when a new retention lease is added. A follow-up commit will add a background sync of the retention leases as well so that renewed retention leases are synced to replicas.

When adding a retention lease, we make a reference copy of the retention leases under lock and then make a copy of that collection outside of the lock. However, since we merely copied a reference to the retention leases, after leaving a lock the underlying collection could change on us. Rather, we want to copy these under lock. This commit adds a dedicated method for doing this, asserts that we hold a lock when we use this method, and changes adding a retention lease to use this method. This commit was intended to be included with #37398 but was pushed to the wrong branch.

DaveCTurner · 2019-02-11T11:05:04Z

server/src/main/java/org/elasticsearch/index/seqno/ReplicationTracker.java

        assert primaryMode;
-        retentionLeases.put(id, new RetentionLease(id, retainingSequenceNumber, currentTimeMillisSupplier.getAsLong(), source));
+        if (retentionLeases.containsKey(id) == false) {
+            throw new IllegalArgumentException("retention lease with ID [" + id + "] does not exist");


I have a concern about the strictness introduced here. I think clients of this API are generally going to be trying to ensure that there is a retention lease in place, and I worry about a situation where, for example, the system clock were to jump forward far enough for all existing leases to expire. I think it would be good in that case if clients were to re-establish these expired leases, but today they would be rejected by this method which means that clients will generally have to be prepared to call addRetentionLease if they discover that their lease has expired. I think it'd be better if we implemented that fall-back behaviour here where clients can't forget to call it (and where it can be correctly synchronised).

I think this concern is a good one and worth discussing in a larger group. You can see what it presents for consumers in the current integration of CCR with shard history retention leases (WIP branch on my fork).

That said, I am not sure how much to worry about this in practice. In most cases, the life of a retention lease should be significantly longer than the renewal period (e.g., default retention of twelve hours versus renewal frequencies of at most sixty seconds in the case of a CCR follower). And also longer than what most clocks jump by (e.g., for daylight savings time) except when they are completely misconfigured and then we have no guarantees.

Introduce retention lease syncing

1deaf16

This commit introduces retention lease syncing from the primary to its replicas when a new retention lease is added. A follow-up commit will add a background sync of the retention leases as well so that renewed retention leases are synced to replicas.

jasontedor added >enhancement v7.0.0 :Distributed/Distributed A catch all label for anything in the Distributed Area. If you aren't sure, use this one. v6.7.0 labels Jan 14, 2019

jasontedor requested review from martijnvg, bleskes, ywelsch and dnhatn January 14, 2019 04:18

jasontedor mentioned this pull request Jan 14, 2019

Shard history retention leases #37165

Closed

24 tasks

Fix toString

a7eb62f

dnhatn reviewed Jan 14, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/seqno/RetentionLeaseSyncAction.java Show resolved Hide resolved

dnhatn reviewed Jan 14, 2019

View reviewed changes

server/src/test/java/org/elasticsearch/index/seqno/RetentionLeaseSyncIT.java Outdated Show resolved Hide resolved

bleskes reviewed Jan 14, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/seqno/ReplicationTracker.java Outdated Show resolved Hide resolved

server/src/main/java/org/elasticsearch/index/seqno/ReplicationTracker.java Outdated Show resolved Hide resolved

jasontedor commented Jan 14, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/shard/IndexShard.java Show resolved Hide resolved

This was referenced Jan 14, 2019

Add run under primary permit method #37440

Merged

Introduce retention lease serialization #37447

Merged

Add simple method to write collection of writeables #37448

Merged

jasontedor added 8 commits January 14, 2019 21:08

Fix test bug

184874e

Merge branch 'master' into sync-retention-leases

e20143c

* master: Add simple method to write collection of writeables (elastic#37448) Fix retention lease commit test

Merge branch 'master' into sync-retention-leases

8947835

* master: Reformat some classes in the index universe [DOCS] Add watcher context examples (elastic#36565)

Merge branch 'master' into sync-retention-leases

6a10869

* master: Add run under primary permit method (elastic#37440)

Run under permit

d6620ec

Do not hold a lock on callback

3d75772

Revert unneeded permit

11a8406

jasontedor added 6 commits January 24, 2019 23:08

Fix imports

50df0db

Remove unnecessary throws

35850f4

Javadocs

fff8763

Add security privileges

9413bf7

Flush on new lease

42ab377

Add comment

54f2511

jasontedor requested review from bleskes and dnhatn January 25, 2019 22:25

jasontedor added 2 commits January 25, 2019 17:33

Add assertion

09fcc7c

jasontedor commented Jan 26, 2019

View reviewed changes

server/src/test/java/org/elasticsearch/index/seqno/RetentionLeaseSyncActionTests.java Outdated Show resolved Hide resolved

jasontedor added 4 commits January 25, 2019 19:13

Update server/src/test/java/org/elasticsearch/index/seqno/RetentionLe…

202922b

…aseSyncActionTests.java

Move comment

df30d48

Fix typo and method name

3ec870d

Rename

13465a2

Remove newline

4a28ebf

dnhatn approved these changes Jan 26, 2019

View reviewed changes

jasontedor merged commit 5fddb63 into elastic:master Jan 27, 2019

jasontedor deleted the sync-retention-leases branch January 27, 2019 13:28

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

DaveCTurner reviewed Feb 11, 2019

View reviewed changes

mkleen mentioned this pull request Feb 19, 2021

bp: Migrate peer recovery from translog to retention lease crate/crate#11044

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce retention lease syncing #37398

Introduce retention lease syncing #37398

jasontedor commented Jan 14, 2019

elasticmachine commented Jan 14, 2019

jasontedor commented Jan 14, 2019

jasontedor commented Jan 14, 2019

dnhatn left a comment

dnhatn Jan 14, 2019

bleskes left a comment

jasontedor commented Jan 25, 2019

jasontedor commented Jan 26, 2019

jasontedor commented Jan 26, 2019

dnhatn left a comment

dnhatn Jan 26, 2019

jasontedor Jan 26, 2019

dnhatn Jan 26, 2019

jasontedor Jan 27, 2019

dnhatn commented Jan 26, 2019 •

edited

Loading

DaveCTurner Feb 11, 2019

jasontedor Feb 11, 2019

Introduce retention lease syncing #37398

Introduce retention lease syncing #37398

Conversation

jasontedor commented Jan 14, 2019

elasticmachine commented Jan 14, 2019

jasontedor commented Jan 14, 2019

jasontedor commented Jan 14, 2019

dnhatn left a comment

Choose a reason for hiding this comment

dnhatn Jan 14, 2019

Choose a reason for hiding this comment

bleskes left a comment

Choose a reason for hiding this comment

jasontedor commented Jan 25, 2019

jasontedor commented Jan 26, 2019

jasontedor commented Jan 26, 2019

dnhatn left a comment

Choose a reason for hiding this comment

dnhatn Jan 26, 2019

Choose a reason for hiding this comment

jasontedor Jan 26, 2019

Choose a reason for hiding this comment

dnhatn Jan 26, 2019

Choose a reason for hiding this comment

jasontedor Jan 27, 2019

Choose a reason for hiding this comment

dnhatn commented Jan 26, 2019 • edited Loading

DaveCTurner Feb 11, 2019

Choose a reason for hiding this comment

jasontedor Feb 11, 2019

Choose a reason for hiding this comment

dnhatn commented Jan 26, 2019 •

edited

Loading