Use Lucene soft-deletes in peer recovery #30522

dnhatn · 2018-05-10T21:03:59Z

This commit adds Lucene soft-deletes as another source for peer-recovery besides translog.

We are targeting to replace translog by Lucene history in peer-recovery. In peer-recovery, the primary transfers a safe commit to replicas then sends subsequent operations after the local checkpoint of that safe commit. This requires the primary having all operations after the checkpoint of the current safe commit. This commit hardens the soft-deletes retention merge policy by taking safe-commit into account.

elasticmachine · 2018-05-10T21:04:01Z

Pinging @elastic/es-distributed

dnhatn · 2018-05-10T22:51:48Z

@elasticmachine test this please

s1monw

I left some questions

s1monw · 2018-05-14T13:51:39Z

server/src/test/java/org/elasticsearch/indices/stats/IndexStatsIT.java

@@ -1049,6 +1049,7 @@ public void testFilterCacheStats() throws Exception {
        assertEquals(DocWriteResponse.Result.DELETED, client().prepareDelete("index", "type", "2").get().getResult());
        if (IndexSettings.INDEX_SOFT_DELETES_SETTING.get(settings)) {
            persistGlobalCheckpoint("index"); // Need to persist the global checkpoint for the soft-deletes retention MP.
+            forceMerge();


why is this needed? can we randomly do it?

s1monw · 2018-05-14T13:52:29Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

+        if (primary.indexSettings().isSoftDeleteEnabled()) {
+            // Need to sync global checkpoint and create a new safe commit as the soft-deletes MergePolicy depends on it.
+            primary.flush(new FlushRequest());
+            primary.forceMerge(new ForceMergeRequest().onlyExpungeDeletes(true));


why is this needed? the force merge seems not necessary maybe do it randomly

Yeah, I was wrong - a flush is enough. I pushed 9ae627c.

s1monw · 2018-05-14T13:57:30Z

server/src/main/java/org/elasticsearch/index/engine/SoftDeletesPolicy.java

+    private final LongSupplier globalCheckpointSupplier;
+    private int retentionLockCount;
+    private long checkpointOfSafeCommit;
+    private volatile long minRequiredSeqNoForRecovery;


just make all methods synchronized then we don't need to do the volatile thing here. the contention on these locks seems very low.

I pushed 176b497

s1monw · 2018-05-14T13:57:51Z

server/src/main/java/org/elasticsearch/index/engine/SoftDeletesPolicy.java

+     */
+    synchronized Releasable acquireRetentionLock() {
+        retentionLockCount++;
+        assert retentionLockCount > 0 : "Invalid number of retention locks [" + retentionLockCount + "]";


if this is needed I think the assertion should happen before we mutate?

dnhatn · 2018-05-14T15:41:03Z

@simonw I've addressed your comments. Can you please take another look? Thank you!

s1monw

left some minors. I think @bleskes should also look this. LGTM

s1monw · 2018-05-14T19:34:14Z

server/src/main/java/org/elasticsearch/index/engine/SoftDeletesPolicy.java

+    // Package-level for testing
+    synchronized long getMinSeqNoToRetain() {
+        final long minSeqNoForQueryingChanges = globalCheckpointSupplier.getAsLong() + 1 - retentionOperations;
+        return Math.min(minRequiredSeqNoForRecovery, minSeqNoForQueryingChanges);


instead of adding the 1 on assignment which is done in 2 places should we do it here?

s1monw · 2018-05-14T19:36:07Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

@@ -2919,7 +2917,12 @@ public void testSegmentMemoryTrackedInBreaker() throws Exception {

        // Deleting a doc causes its memory to be freed from the breaker
        deleteDoc(primary, "_doc", "0");
-        primary.sync(); // need to sync global checkpoint as the soft-deletes retention MergePolicy depends on it.
+        if (primary.indexSettings().isSoftDeleteEnabled()) {


not sure about this I think @bleskes needs to look at this

s1monw · 2018-05-14T19:36:22Z

server/src/test/java/org/elasticsearch/indices/stats/IndexStatsIT.java

-            persistGlobalCheckpoint("index"); // Need to persist the global checkpoint for the soft-deletes retention MP.
+            // Need to persist the global checkpoint and flush a new safe commit
+            // as the soft-deletes retention merge policy depends on the local-checkpoint of the safe commit.
+            persistGlobalCheckpoint("index");


not sure about this I think @bleskes needs to look at this

I don't follow the comment - @dnhatn can you please explain?

I added a comment for this.

bleskes

I left some comments. I would also like to understand (and see a test) how this interact with ongoing merges that have started before the safe commit was updated and thus use old information (I think it's OK but we need to know for sure). This PR tries to guarantee that all operations above the commit's local checkpoint will be available in any future NRT reader - that may include the result of such a merge.

bleskes · 2018-05-15T10:04:07Z

server/src/main/java/org/elasticsearch/index/engine/SoftDeletesPolicy.java

+    private void updateMinRequiredSeqNoForRecovery() {
+        assert Thread.holdsLock(this) : Thread.currentThread().getName();
+        if (retentionLockCount == 0) {
+            this.minRequiredSeqNoForRecovery = checkpointOfSafeCommit;


name this storeRecovery? I think it will help clarify that this is only used for store recovery.

bleskes · 2018-05-15T13:11:11Z

server/src/test/java/org/elasticsearch/indices/stats/IndexStatsIT.java

-            persistGlobalCheckpoint("index"); // Need to persist the global checkpoint for the soft-deletes retention MP.
+            // Need to persist the global checkpoint and flush a new safe commit
+            // as the soft-deletes retention merge policy depends on the local-checkpoint of the safe commit.
+            persistGlobalCheckpoint("index");


I don't follow the comment - @dnhatn can you please explain?

bleskes · 2018-05-15T13:11:41Z

server/src/test/java/org/elasticsearch/index/shard/IndexShardTests.java

-        primary.sync(); // need to sync global checkpoint as the soft-deletes retention MergePolicy depends on it.
+        if (primary.indexSettings().isSoftDeleteEnabled()) {
+            // Need to persist the global checkpoint and flush a new safe commit
+            // as the soft-deletes retention merge policy depends on the local-checkpoint of the safe commit.


same here. Can you please explain more?

I added a comment for this.

server/src/main/java/org/elasticsearch/index/engine/SoftDeletesPolicy.java

+     * make sure that all operations after the local checkpoint of the safe commit are retained until the lock is released.
+     * This is a analogy to the translog's retention lock; see {@link Translog#acquireRetentionLock()}
+     */
+    synchronized Releasable acquireRetentionLock() {


dnhatn · 2018-05-16T02:57:06Z

@bleskes and @s1monw
I took a closer look at the retention lock. Unfortunately, it does not work as expected. I explained this at
https://github.com/elastic/elasticsearch/pull/30522/files#diff-9feb4947ba4df0b486c7de6b9d46502fR149. Please have a look, thank you!

I've included the peer-recovery usage in this PR so that we can have the whole picture of this change. I will continue working on tests.

s1monw · 2018-06-13T12:59:17Z

Looks great IMO

s1monw · 2018-06-15T14:04:51Z

@bleskes this is still waiting on you

bleskes

This looks great. My only concern is that we should randomize the usage of soft deletes in our standard testing. We don't plan to turn it on by default for 6.x and we should make sure everything works without it as well as with.

bleskes · 2018-06-19T12:08:24Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

@@ -257,6 +257,18 @@ private LocalCheckpointTracker createLocalCheckpointTracker(
        return localCheckpointTrackerSupplier.apply(maxSeqNo, localCheckpoint);
    }

+    private SoftDeletesPolicy newSoftDeletesPolicy() throws IOException {
+        final Map<String, String> commitUserData = store.readLastCommittedSegmentsInfo().userData;
+        final long lastSeqNoSeenByMergePolicy;


nit: rename to minRetainedSeqNo ?

bleskes · 2018-06-19T12:08:36Z

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java

@@ -468,18 +480,39 @@ public void syncTranslog() throws IOException {
    }

    @Override
-    public Closeable acquireTranslogRetentionLock() {
-        return getTranslog().acquireRetentionLock();
+    public Translog.Snapshot newTranslogSnapshotBetween(long minSeqNo, long maxSeqNo) throws IOException {


I think we can remove this as a follow up, in favor of readHistoryOperations ?

Yes, we will remove this method after making primary-replica resync use Lucene.

bleskes · 2018-06-19T12:17:25Z

server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

+        store = createStore();
+        engine = createEngine(config(indexSettings, store, createTempDir(), newMergePolicy(), null, null, globalCheckpoint::get));
+        assertThat(engine.getMinRetainedSeqNo(), equalTo(0L));
+        long lastSeqNoSeenByMP = engine.getMinRetainedSeqNo();


nit: rename

dnhatn · 2018-06-19T12:31:40Z

My only concern is that we should randomize the usage of soft deletes in our standard testing. We don't plan to turn it on by default for 6.x and we should make sure everything works without it as well as with.

@bleskes Once this PR is in, I will make a follow-up to randomize the soft-deletes setting? Are you okay?

bleskes · 2018-06-19T12:40:51Z

I will make a follow-up to randomize the soft-deletes setting? Are you okay?

Of course.

dnhatn · 2018-06-20T14:47:58Z

This PR is blocked by #31442; I will merge once #31442 is in.

dnhatn · 2018-06-21T03:03:48Z

@elasticmachine test this please

dnhatn · 2018-06-21T13:29:59Z

Thanks @s1monw and @bleskes for reviewing!

This commit adds Lucene soft-deletes as another source for peer-recovery besides translog. Relates #29530

Today a file-based recovery will replay all existing translog operations from the primary on a replica so that that replica can have a full history in translog as the primary. However, with soft-deletes enabled, we should not do it because: 1. All operations before the local checkpoint of the safe commit exist in the commit already. 2. The number of operations before the local checkpoint may be considerable and requires a significant amount of time to replay on a replica. Relates elastic#30522 Relates elastic#29530

…es (#33190) Today a file-based recovery will replay all existing translog operations from the primary on a replica so that that replica can have a full history in translog as the primary. However, with soft-deletes enabled, we should not do it because: 1. All operations before the local checkpoint of the safe commit exist in the commit already. 2. The number of operations before the local checkpoint may be considerable and requires a significant amount of time to replay on a replica. Relates #30522 Relates #29530

This PR integrates Lucene soft-deletes (LUCENE-8200) into Elasticsearch. Highlight works in this PR include: 1. Replace hard-deletes by soft-deletes in InternalEngine 2. Use _recovery_source if _source is disabled or modified (elastic#31106) 3. Soft-deletes retention policy based on the global checkpoint (elastic#30335) 4. Read operation history from Lucene instead of translog (elastic#30120) 5. Use Lucene history in peer-recovery (elastic#30522) These works have been done by the whole team; however, these individuals (lexical order) have significant contribution in coding and reviewing: Co-authored-by: Adrien Grand <[email protected]> Co-authored-by: Boaz Leskes <[email protected]> Co-authored-by: Jason Tedor <[email protected]> Co-authored-by: Martijn van Groningen <[email protected]> Co-authored-by: Nhat Nguyen <[email protected]> Co-authored-by: Simon Willnauer <[email protected]>

This PR integrates Lucene soft-deletes(LUCENE-8200) into Elasticsearch. Highlight works in this PR include: - Replace hard-deletes by soft-deletes in InternalEngine - Use _recovery_source if _source is disabled or modified (#31106) - Soft-deletes retention policy based on the global checkpoint (#30335) - Read operation history from Lucene instead of translog (#30120) - Use Lucene history in peer-recovery (#30522) Relates #30086 Closes #29530 --- These works have been done by the whole team; however, these individuals (lexical order) have significant contribution in coding and reviewing: Co-authored-by: Adrien Grand [email protected] Co-authored-by: Boaz Leskes [email protected] Co-authored-by: Jason Tedor [email protected] Co-authored-by: Martijn van Groningen [email protected] Co-authored-by: Nhat Nguyen [email protected] Co-authored-by: Simon Willnauer [email protected]

This PR integrates Lucene soft-deletes(LUCENE-8200) into Elasticsearch. Highlight works in this PR include: - Replace hard-deletes by soft-deletes in InternalEngine - Use _recovery_source if _source is disabled or modified (elastic#31106) - Soft-deletes retention policy based on the global checkpoint (elastic#30335) - Read operation history from Lucene instead of translog (elastic#30120) - Use Lucene history in peer-recovery (elastic#30522) Relates elastic#30086 Closes elastic#29530 --- These works have been done by the whole team; however, these individuals (lexical order) have significant contribution in coding and reviewing: Co-authored-by: Adrien Grand <[email protected]> Co-authored-by: Boaz Leskes <[email protected]> Co-authored-by: Jason Tedor <[email protected]> Co-authored-by: Martijn van Groningen <[email protected]> Co-authored-by: Nhat Nguyen <[email protected]> Co-authored-by: Simon Willnauer <[email protected]>

This PR integrates Lucene soft-deletes(LUCENE-8200) into Elasticsearch. Highlight works in this PR include: - Replace hard-deletes by soft-deletes in InternalEngine - Use _recovery_source if _source is disabled or modified (#31106) - Soft-deletes retention policy based on the global checkpoint (#30335) - Read operation history from Lucene instead of translog (#30120) - Use Lucene history in peer-recovery (#30522) Relates #30086 Closes #29530 --- These works have been done by the whole team; however, these individuals (lexical order) have significant contribution in coding and reviewing: Co-authored-by: Adrien Grand <[email protected]> Co-authored-by: Boaz Leskes <[email protected]> Co-authored-by: Jason Tedor <[email protected]> Co-authored-by: Martijn van Groningen <[email protected]> Co-authored-by: Nhat Nguyen <[email protected]> Co-authored-by: Simon Willnauer <[email protected]>

dnhatn added >feature :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels May 10, 2018

dnhatn requested review from s1monw and bleskes May 10, 2018 21:04

Don’t check local_checkpoint in commit

4b2e385

s1monw reviewed May 14, 2018

View reviewed changes

dnhatn added 2 commits May 14, 2018 11:13

Prefer synchronized methods

176b497

Remove force-merge

9ae627c

dnhatn requested a review from s1monw May 14, 2018 15:41

s1monw approved these changes May 14, 2018

View reviewed changes

dnhatn added 2 commits May 14, 2018 16:00

Merge branch 'ccr' into safecommit-mergepolicy

ff2215c

Add +1 in 1 place

188138d

bleskes suggested changes May 15, 2018

View reviewed changes

dnhatn added 7 commits May 15, 2018 13:46

Merge branch 'ccr' into safecommit-mergepolicy

6d901bf

acquireTranslogRetentionLock -> acquireRetentionLockForPeerRecovery

8a78f65

Cut over the translog snapshot

6fe8847

testRecoveryWithOutOfOrderDelete with Lucene history

cc2b3f0

Adapt PrimaryReplicaSyncerTests test

0612a05

Mute two more tests

a61d00b

Add discuss

f3f1fa2

dnhatn added the WIP label May 16, 2018

comment

65b8458

dnhatn changed the title ~~Introduce soft-deletes retention policy for peer recovery~~ Use Lucene soft-deletes in peer recovery May 16, 2018

dnhatn added 2 commits May 15, 2018 23:52

Update testRecoveryWithOutOfOrderDelete

fc3d7d1

Minor feedback

bd1b8ac

Merge branch 'ccr' into safecommit-mergepolicy

8a37126

Merge branch 'ccr' into safecommit-mergepolicy

88950b3

bleskes suggested changes Jun 19, 2018

View reviewed changes

bleskes approved these changes Jun 19, 2018

View reviewed changes

dnhatn added 2 commits June 19, 2018 12:07

Merge branch 'ccr' into safecommit-mergepolicy

dbe0472

Naming

f9eeb90

Merge branch 'ccr' into safecommit-mergepolicy

ccb6e80

dnhatn merged commit 601ea76 into elastic:ccr Jun 21, 2018

dnhatn deleted the safecommit-mergepolicy branch June 21, 2018 13:30

dnhatn added the backport pending label Jun 21, 2018

dnhatn mentioned this pull request Jun 21, 2018

Use soft-deletes to maintain document history #29530

Closed

14 tasks

dnhatn added a commit that referenced this pull request Jun 24, 2018

Use Lucene soft-deletes in peer recovery (#30522)

8de4659

This commit adds Lucene soft-deletes as another source for peer-recovery besides translog. Relates #29530

dnhatn removed the backport pending label Jun 28, 2018

dnhatn mentioned this pull request Aug 28, 2018

File-based recovery with soft-deletes should send only operations after the local checkpoint #33190

Merged

dnhatn mentioned this pull request Aug 29, 2018

Integrates soft-deletes into Elasticsearch #33222

Merged

Use Lucene soft-deletes in peer recovery #30522

Use Lucene soft-deletes in peer recovery #30522

Conversation

dnhatn commented May 10, 2018 • edited Loading

elasticmachine commented May 10, 2018

dnhatn commented May 10, 2018

s1monw 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

dnhatn commented May 14, 2018

s1monw 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

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

This comment was marked as resolved.

This comment was marked as resolved.

dnhatn commented May 16, 2018

s1monw commented Jun 13, 2018

s1monw commented Jun 15, 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

dnhatn commented Jun 19, 2018

bleskes commented Jun 19, 2018

dnhatn commented Jun 20, 2018

dnhatn commented Jun 21, 2018

dnhatn commented Jun 21, 2018

dnhatn commented May 10, 2018 •

edited

Loading