Add listener for background lease sync completion #39240

DaveCTurner · 2019-02-21T13:46:08Z

Today the background sync of retention leases is a fire-and-forget action, and
therefore so is IndexShard#syncRetentionLeases(). However, in tests sometimes
we want to sync the retention leases and then wait for its completion before
continuing, and today this wait has to be a busy-wait. This change allows us to
listen for completion instead.

Today the background sync of retention leases is a fire-and-forget action, and therefore so is `IndexShard#syncRetentionLeases()`. However, in tests sometimes we want to sync the retention leases and then wait for its completion before continuing, and today this wait has to be a busy-wait. This change allows us to listen for completion instead.

elasticmachine · 2019-02-21T13:46:32Z

Pinging @elastic/es-distributed

DaveCTurner · 2019-02-21T13:49:14Z

This change is groundwork for the introduction of peer-recovery retention leases: if history retention is to be coordinated by the primary we must explicitly sync the leases to release history in a reasonable number of other tests.

I.e. relates #39133.

dnhatn

I left a question but this looks good.

dnhatn · 2019-02-21T22:14:05Z

server/src/main/java/org/elasticsearch/index/IndexService.java


    private void syncRetentionLeases() {
-        sync(IndexShard::syncRetentionLeases, "retention lease");
+        sync(primary -> primary.syncRetentionLeases(wrap(() -> {})), "retention lease");


maybe just use ActionListener.wrap without a static import?

dnhatn · 2019-02-21T22:32:14Z

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

     * @param listener the callback when the retention lease is successfully removed and synced to replicas
     */
-    public void removeRetentionLease(final String id, final ActionListener<ReplicationResponse> listener) {
+    public void removeRetentionLease(final String id, final ActionListener<Void> listener) {


I know we are not really using the response but is there any reason for this change? Is it okay if we make both sync and backgroundSync return ReplicationResponse instead of Void?

Primarily because I think RetentionLeaseSyncer.EMPTY should notify the listener (the contract for a method that accepts an ActionListener is that it is eventually notified, which today we do not satisfy) and yet that implementation does not have a ReplicationResponse with which to notify it. I could make one up, but it felt wrong.

I think we can move RetentionLeaseSyncer.EMPTY to tests and make it notify listeners with an empty ReplicationResponse?

Ok, I reverted back to using a made-up ReplicationResponse in 50acab7; I think I'll leave RetentionLeaseSyncer.EMPTY where it is.

dnhatn · 2019-02-21T22:41:29Z

test/framework/src/main/java/org/elasticsearch/test/InternalTestCluster.java

+        }
+        futures.forEach(f -> {
+            try {
+                f.get();


nit: maybe use actionGet.

dnhatn · 2019-02-21T22:49:10Z

server/src/test/java/org/elasticsearch/index/replication/RetentionLeasesReplicationTests.java

-                group.addRetentionLease(Integer.toString(i), randomNonNegativeLong(), "test-" + i, addLeaseFuture);
-                RetentionLeaseSyncAction.Request request = ((SyncRetentionLeasesResponse) addLeaseFuture.actionGet()).syncRequest;
+            final IndexShard newPrimary = randomFrom(group.getReplicas());
+            final AtomicReference<RetentionLeases> latestRetentionLeasesOnNewPrimary = new AtomicReference<>(RetentionLeases.EMPTY);


This test becomes a bit more tangled since we change to a Void listener.

It does. I nearly backed out the whole idea because of this test. OTOH it has less casting after this change.

jasontedor

This is a change to production code to support testing. Sometimes that is the right thing. Yet, I am not following enough the thinking to make this change. Can you explain more why assert busy is not enough for these cases? Since in production these things are only going to eventually happen, I feel like our tests should reflect that rather than making them artificial with an actual sync point that would never exist in production code?

I am marking this as "Request changes" only to be clear that I would like to have a better understanding for this change before it is merged.

DaveCTurner · 2019-02-22T09:35:56Z

We have a number of tests that perform some other "happens eventually" actions in a synchronous fashion (refreshes/flushes) and then assert that history has been discarded or segments have been deleted. Although technically we're asserting that history has been discarded only if the user performed an explicit refresh/flush in those cases, what we're trying to ensure is really that history is discarded eventually even without that explicit action. I think this is a reasonable extrapolation, but if we're not happy with this then we should consider moving these tests to assertBusy() on the refresh/flush having happened too.

I am not a huge fan of assertBusy(). There are places where we have to use it, but it leads to slower tests that rely on timely activity that CI can't always deliver, and can be avoided here with a fairly small exposed-for-tests change to the API of the RetentionLeaseSyncer interface that itself maybe wouldn't exist if we didn't need it for tests.

…eners

DaveCTurner · 2019-02-28T07:51:15Z

@elasticmachine please run elasticsearch-ci/docbldesx

dnhatn

This looks great. Thanks @DaveCTurner.

@jasontedor Are you okay with this change?

dnhatn · 2019-02-28T18:24:55Z

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


        // now ensure that the retention leases are the same
-        assertBusy(() -> {
+        {


I think we need to call syncRetentionLeases or keep assertBusy.

I think it's sufficient not to do so because we are asserting that leases were not renewed, so checking the primary is enough. But it does not cause any harm, so I added an explicit sync in fc4459e.

…eners

DaveCTurner · 2019-03-19T12:45:59Z

@jasontedor it'd be good to resolve this one way or another - could you let me know your thoughts?

jasontedor · 2019-03-30T22:12:27Z

@DaveCTurner I still feel how I felt when I left my initial comment. That is, I understand this cleans up some assertBusys and makes some tests easier to reason about, but I still have concerns about tests running code that is a little bit different than what production code would do.

As I went back and read this thread again, and read the implementation, I saw a comment that I missed before.

This change is groundwork for the introduction of peer-recovery retention leases: if history retention is to be coordinated by the primary we must explicitly sync the leases to release history in a reasonable number of other tests.

I am not following this, can you elaborate?

DaveCTurner · 2019-04-25T14:34:17Z

On reflection this isn't obviously necessary for peer recovery retention leases: I think I can use assertBusy wherever necessary.

DaveCTurner added v7.0.0 v8.0.0 v7.2.0 labels Feb 21, 2019

DaveCTurner requested review from dnhatn and jasontedor February 21, 2019 13:46

DaveCTurner added >non-issue :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. :Distributed Indexing/CCR Issues around the Cross Cluster State Replication features labels Feb 21, 2019

DaveCTurner added the v6.7.0 label Feb 21, 2019

Imports

5336f11

dnhatn reviewed Feb 21, 2019

View reviewed changes

jasontedor requested changes Feb 22, 2019

View reviewed changes

DaveCTurner added 9 commits February 25, 2019 08:19

Merge branch 'master' into 2019-02-21-sync-retention-leases-with-list…

f54f462

…eners

Lost import

8cb7a8a

Review feedback

c3414d1

Only rarely rely on periodic sync in wait-for-active-shards tests

4a9e605

Sync leases explicitly rather than sleeping

562b951

Imports

7bf783b

Merge branch 'master' into 2019-02-21-sync-retention-leases-with-list…

1ced325

…eners

Merge fixup

b067cde

Imports

2e0e555

DaveCTurner requested review from jasontedor and ywelsch February 27, 2019 16:38

DaveCTurner added 2 commits February 27, 2019 17:23

Revert API change ReplicationResponse -> Void

50acab7

Assert listener is invoked

fae390e

DaveCTurner requested a review from dnhatn February 27, 2019 17:31

Merge branch 'master' into 2019-02-21-sync-retention-leases-with-list…

4922739

…eners

dnhatn approved these changes Feb 28, 2019

View reviewed changes

DaveCTurner added 3 commits March 1, 2019 08:25

Explicit sync

fc4459e

Merge branch 'master' into 2019-02-21-sync-retention-leases-with-list…

b30da8c

…eners

Fixup compile error

f97a145

danielmitterdorfer added v6.7.1 and removed v6.7.0 labels Mar 21, 2019

colings86 added v6.7.2 and removed v6.7.1 labels Mar 30, 2019

$@polyfractal$ polyfractal removed the v7.0.0 label Apr 9, 2019

colings86 added v6.7.3 and removed v6.7.2 labels Apr 17, 2019

DaveCTurner closed this Apr 25, 2019

DaveCTurner deleted the 2019-02-21-sync-retention-leases-with-listeners branch April 25, 2019 14:34

colings86 added v6.7.2 and removed v6.7.3 labels Apr 26, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Add listener for background lease sync completion #39240

Add listener for background lease sync completion #39240

Uh oh!

Conversation

DaveCTurner commented Feb 21, 2019

Uh oh!

elasticmachine commented Feb 21, 2019

Uh oh!

DaveCTurner commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dnhatn left a comment

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!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

DaveCTurner commented Feb 22, 2019

Uh oh!

DaveCTurner commented Feb 28, 2019

Uh oh!

dnhatn left a comment

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!

DaveCTurner commented Mar 19, 2019

Uh oh!

jasontedor commented Mar 30, 2019

Uh oh!

DaveCTurner commented Apr 25, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

DaveCTurner commented Feb 21, 2019 •

edited

Loading