Keep commits and translog up to the global checkpoint #27606

dnhatn · 2017-11-30T14:37:29Z

I have to include both index commit and translog policies in a single PR because they need one another for testing. This PR is a rework of #27367.

We need to keep index commits and translog operations up to the current global checkpoint to allow us to throw away unsafe operations and increase the operation-based recovery chance. This is achieved by a new index deletion policy.

DaveCTurner

Thanks @dnhatn - I left some small feedback points.

DaveCTurner · 2017-12-01T13:09:28Z

core/src/main/java/org/elasticsearch/common/lucene/index/ESIndexDeletionPolicy.java

+     * @return a list of index commits that are not deleted by this policy.
+     */
+    List<IndexCommit> onCommit(List<? extends IndexCommit> commits) throws IOException;
+}


This interface clearly came from IndexDeletionPolicy but I think it's now different enough that it's worth breaking the link entirely and coming up with more descriptive names. The onInit() and onCommit() methods of IndexDeletionPolicy return void but here we're returning a list of the kept commits. Additionally, I think all the implementations just delegate onInit() to onCommit() (except KeepUntilGlobalCheckpointDeletionPolicy that special-cases an empty argument).

DaveCTurner · 2017-12-01T13:09:59Z

core/src/main/java/org/elasticsearch/common/lucene/index/ESIndexDeletionPolicy.java

+import java.util.List;
+
+/**
+ * An {@link IndexDeletionPolicy} that deletes unneeded index commits, and returns index commits are not deleted by this policy.


Not an IndexDeletionPolicy any more!

DaveCTurner · 2017-12-01T13:12:33Z

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

+            case OPEN_INDEX_AND_TRANSLOG:
+                final long globalCheckpoint = Translog.readGlobalCheckpoint(engineConfig.getTranslogConfig().getTranslogPath());
+                seqNoStats = store.loadSeqNoStats(globalCheckpoint);
+                break;


I'd just return store.loadSeqNoStats() here rather than breaking and returning at the bottom. Not sure if that's against our style tho?

DaveCTurner · 2017-12-01T13:14:47Z

core/src/main/java/org/elasticsearch/index/engine/KeepUntilGlobalCheckpointDeletionPolicy.java

+
+/**
+ * An {@link ESIndexDeletionPolicy} that deletes index commits that are not required for recovery.
+ * In particular, this policy will delete index commits whose max sequence number is smaller (or equal) than


s/smaller (or equal) than/at most/ ?

DaveCTurner · 2017-12-01T13:21:01Z

core/src/test/java/org/elasticsearch/index/translog/TranslogTests.java

-        commit(translog, generation);
-    }
+        long lastGen = randomLongBetween(1, translog.currentFileGeneration());
+        commit(translog, randomLongBetween(1, lastGen), lastGen);    }


nit: } should be on its own line

DaveCTurner · 2017-12-01T13:25:14Z

core/src/main/java/org/elasticsearch/index/engine/KeepUntilGlobalCheckpointDeletionPolicy.java

+
+        final List<IndexCommit> keptCommits = new ArrayList<>();
+        final int keptPosition = indexOfKeptCommits(commits);
+        final List<Integer> duplicateIndexes = indexesOfDuplicateCommits(commits);


We only call this collection's contains() method so maybe a HashSet<Integer> would be better? Not sure if it gets big enough to make a difference.

We expect to have zero or one element in a list, there should be no difference. However, using Set makes more sense here; I updated. Thank you.

DaveCTurner · 2017-12-01T13:30:34Z

core/src/test/java/org/elasticsearch/indices/recovery/RecoveryTests.java

@@ -113,27 +113,10 @@ public void testRecoveryWithOutOfOrderDelete() throws Exception {
            orgReplica.applyIndexOperationOnReplica(3, 1, VersionType.EXTERNAL, IndexRequest.UNSET_AUTO_GENERATED_TIMESTAMP, false,
                SourceToParse.source(orgReplica.shardId().getIndexName(), "type", "id2", new BytesArray("{}"), XContentType.JSON), u -> {});

-            final int translogOps;
+            final int translogOps = 4; // 3 ops + seqno gaps


This removes the tests for the cases where we've updated the index settings. Is that ok?

I am working to have another test for this as the assumption in this test is no longer correct.

which other test is that?

ywelsch · 2017-12-01T14:05:34Z

core/src/main/java/org/elasticsearch/common/lucene/index/ESIndexDeletionPolicy.java

+/**
+ * An {@link IndexDeletionPolicy} that deletes unneeded index commits, and returns index commits are not deleted by this policy.
+ */
+public interface ESIndexDeletionPolicy {


do we need a separate interface? If we want to know which commits can be kept,
we can just do commits.stream().allMatch(c -> c.isDeleted() == false) after invoking the onInit or onCommit methods?

This interface is to avoid keeping translog of snapshotted index commits. SnapshotDeletionPolicy suppresses the delete() for snapshotted commits.

https://github.com/apache/lucene-solr/blob/e2521b2a8baabdaf43b92192588f51e042d21e97/lucene/core/src/java/org/apache/lucene/index/SnapshotDeletionPolicy.java#L221-L225

I should have documented its purpose.

bleskes · 2017-12-01T21:27:13Z

@dnhatn I have a question before diving deeper - I expected to find CombinedDeletionPolicy merged with KeepUntilGlobalCheckpointDeletionPolicy into a single class. The hunch was that that will removed the need to look at the index commits to know if there were deleted by another policy, allowing that information to be directly transferred to the translog deletion policy. Did that not work for some reason?

dnhatn · 2017-12-01T23:07:29Z

I like the current design, three policies are decoupled; I tried to keep them separated as before. I will embed KeepUntilGlobalCheckpointDeletionPolicy code to the CombinedDeletionPolicy.

dnhatn · 2017-12-03T01:44:48Z

I expected to find CombinedDeletionPolicy merged with KeepUntilGlobalCheckpointDeletionPolicy into a single class

@bleskes, I have merged the KeepUntilGlobalCheckpointDeletionPolicy with CombinedDeletionPolicy. Could you please take a look? Thank you.

dnhatn · 2017-12-03T04:07:22Z

@DaveCTurner I've addressed your comments but not all as I've removed the new interface.

bleskes · 2017-12-03T14:47:14Z

I like the current design, three policies are decoupled; I tried to keep them separated as before. I will embed KeepUntilGlobalCheckpointDeletionPolicy code to the CombinedDeletionPolicy.

I also find the composition of deletion policies elegant. Sadly elegance is an all or nothing thing. Adding extra wraps or copy interface means you lose it...

@bleskes, I have merged the KeepUntilGlobalCheckpointDeletionPolicy with CombinedDeletionPolicy. Could you please take a look? Thank you.

Thanks! I will look tomorrow morning.

DaveCTurner · 2017-12-04T11:10:31Z

Yannick said:

Do we need a separate interface? If we want to know which commits can be kept,
we can just do commits.stream().allMatch(c -> c.isDeleted() == false) after invoking the onInit or onCommit methods?

The interface has gone (👍) and the second half of this idea seems like a good one. The meaning of the return value of deleteOldIndexCommits() requires reading the comments/implementation as it's not clear from the name, whereas commits.stream().allMatch(c -> c.isDeleted() == false) is transparent.

bleskes

I left a question about a potential simpler approach. Also, I see you added some clean up w.r.t to duplicate commits above the global checkpoint with the same max seq no - can you elaborate why this is needed? Can't we just leave them alone?

bleskes · 2017-12-04T13:17:55Z

core/src/main/java/org/elasticsearch/index/engine/CombinedDeletionPolicy.java

    }

    @Override
    public void onInit(List<? extends IndexCommit> commits) throws IOException {
-        indexDeletionPolicy.onInit(commits);
+        final List<IndexCommit> keptCommits = deleteOldIndexCommits(commits);


can we rely on indexOfKeptCommits to give use the index of the first commit to be kept and use that to:

Delete all commits before

Set the translog deletion policy generation based on the commit the index points to.

Yep, I agree. Let's remove the duplicate logic first.

dnhatn · 2017-12-04T14:29:28Z

@bleskes, There are some cases we may end up with multiple index commits having max_seqno.

If a (force) merge is executed, and the global checkpoint is still behind the max_seqno, we will have two commits with the same max_seqno. These two commits may require more disk space. When the global checkpoint catches up the max_seqno, we won't clean up the old commits unless a new commit is created. I added the duplicate logic to avoid this scenario.
A recovering commit from a local store or local shard is immediately replaced by a fresh commit with the same max_seqno. This is not a disk space issue as these two commits should share the same underlying files. The issue is the recovering does not have TRANSLOG_GEN in its commit which requires a special handling when processing translog generation. We can solve this without the duplicate logic.

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

Lines 448 to 468 in 2900e3f

    
           /** If needed, updates the metadata in the index writer to match the potentially new translog and history uuid */ 
        
           private void updateWriterOnOpen() throws IOException { 
        
               Objects.requireNonNull(historyUUID); 
        
               final Map<String, String> commitUserData = commitDataAsMap(indexWriter); 
        
               boolean needsCommit = false; 
        
               if (historyUUID.equals(commitUserData.get(HISTORY_UUID_KEY)) == false) { 
        
                   needsCommit = true; 
        
               } else { 
        
                   assert config().getForceNewHistoryUUID() == false : "config forced a new history uuid but it didn't change"; 
        
                   assert openMode != EngineConfig.OpenMode.CREATE_INDEX_AND_TRANSLOG : "new index but it already has an existing history uuid"; 
        
               } 
        
               if (translog.getTranslogUUID().equals(commitUserData.get(Translog.TRANSLOG_UUID_KEY)) == false) { 
        
                   needsCommit = true; 
        
               } else { 
        
                   assert openMode == EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG : "translog uuid didn't change but open mode is " + openMode; 
        
               } 
        
               if (needsCommit) { 
        
                   commitIndexWriter(indexWriter, translog, openMode == EngineConfig.OpenMode.OPEN_INDEX_CREATE_TRANSLOG 
        
                       ? commitUserData.get(SYNC_COMMIT_ID) : null); 
        
               } 
        
           }

bleskes · 2017-12-04T15:12:50Z

@bleskes, There are some cases we may end up with multiple index commits having max_seqno.

Thanks. Let's keep this a simple as possible. If those duplicate commit become a problem we can fix it later. Note though that when those duplicate commits are the "good ones", we should keep just one of them by choosing the commit appropriately. This is simple as it's just a question of select the commit index correctly.

dnhatn · 2017-12-12T21:22:18Z

@elasticmachine please test this.

bleskes

LGTM. Thanks @dnhatn !

bleskes · 2017-12-12T20:45:03Z

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

+                assert engineConfig.getForceNewHistoryUUID() == false
+                    || openMode == EngineConfig.OpenMode.CREATE_INDEX_AND_TRANSLOG
+                    || openMode == EngineConfig.OpenMode.OPEN_INDEX_CREATE_TRANSLOG
+                    : "OpenMode must be either CREATE_INDEX_AND_TRANSLOG or OPEN_INDEX_CREATE_TRANSLOG if forceNewHistoryUUID is true";


nit: add the current open mode to the message please

dnhatn · 2017-12-12T21:40:22Z

@DaveCTurner Could you please have a look? Thank you.

DaveCTurner

I've lost the thread on this PR, but I requested changes on an earlier version that are either done or superseded.

dnhatn · 2017-12-13T00:18:32Z

Thanks @bleskes and @DaveCTurner.

We need to keep index commits and translog operations up to the current global checkpoint to allow us to throw away unsafe operations and increase the operation-based recovery chance. This is achieved by a new index deletion policy. Relates #10708

The test testWithRandomException was not updated accordingly to the latest translog policy. Method setTranslogGenerationOfLastCommit should be called before whenever setMinTranslogGenerationForRecovery is called. Relates #27606

Today we use the in-memory global checkpoint from SequenceNumbersService to clean up unneeded commit points, however the latest global checkpoint may haven't fsynced to the disk yet. If the translog checkpoint fsync failed and we already use a higher global checkpoint to clean up commit points, then we may have removed a safe commit which we try to keep for recovery. This commit updates the deletion policy using lastSyncedGlobalCheckpoint from Translog rather the in memory global checkpoint. Relates #27606

Today we use the in-memory global checkpoint from SequenceNumbersService to clean up unneeded commit points, however the latest global checkpoint may haven't fsynced to the disk yet. If the translog checkpoint fsync failed and we already use a higher global checkpoint to clean up commit points, then we may have removed a safe commit which we try to keep for recovery. This commit updates the deletion policy using lastSyncedGlobalCheckpoint from Translog rather the in memory global checkpoint. Relates elastic#27606

Currently we keep a 5.x index commit as a safe commit until we have a 6.x safe commit. During that time, if peer-recovery happens, a primary will send a 5.x commit in file-based sync and the recovery will even fail as the snapshotted commit does not have sequence number tags. This commit updates the combined deletion policy to delete legacy commits if there are 6.x commits. Relates elastic#27606 Relates elastic#28038

Currently we keep a 5.x index commit as a safe commit until we have a 6.x safe commit. During that time, if peer-recovery happens, a primary will send a 5.x commit in file-based sync and the recovery will even fail as the snapshotted commit does not have sequence number tags. This commit updates the combined deletion policy to delete legacy commits if there are 6.x commits. Relates #27606 Relates #28038

Keep index commits up to the global checkpoint

ad052ec

We need to keep index commits and translog operations up to the current global checkpoint to allow us to throw away unsafe operations and increase the operation-based recovery chance. This is achieved by a new index deletion policy.

dnhatn added :Sequence IDs >enhancement v6.2.0 v7.0.0 labels Nov 30, 2017

dnhatn requested review from bleskes, ywelsch and jasontedor November 30, 2017 14:37

dnhatn mentioned this pull request Nov 30, 2017

Keeps index commits up to the current global checkpoint #27367

Closed

dnhatn added the review label Nov 30, 2017

DaveCTurner requested changes Dec 1, 2017

View reviewed changes

ywelsch reviewed Dec 1, 2017

View reviewed changes

dnhatn added 4 commits December 1, 2017 13:58

add doc for policy

97e6cff

use set

48381a2

compact switch

4b9ebb6

format

7f32e22

dnhatn added 3 commits December 1, 2017 22:32

add randomLong to ESTestCase

94b735f

move gcp policy to the combined policy

ab262b1

Merge branch 'master' into gcp-deletion-policy

38310cd

bleskes suggested changes Dec 4, 2017

View reviewed changes

Use the kept position

72a4190

bleskes approved these changes Dec 12, 2017

View reviewed changes

dnhatn added 2 commits December 12, 2017 16:36

explain the assertion

09b0f91

Merge branch 'master' into gcp-deletion-policy

ce8b52d

DaveCTurner approved these changes Dec 12, 2017

View reviewed changes

dnhatn merged commit 57fc705 into elastic:master Dec 13, 2017

dnhatn deleted the gcp-deletion-policy branch December 13, 2017 00:20

dnhatn added the backport pending label Dec 13, 2017

dnhatn removed the backport pending label Dec 13, 2017

bleskes mentioned this pull request Dec 13, 2017

Add Sequence Numbers to write operations #10708

Closed

64 tasks

dnhatn mentioned this pull request Dec 14, 2017

Use lastSyncedGlobalCheckpoint in deletion policy #27826

Merged

dnhatn mentioned this pull request Jan 11, 2018

Do not keep 5.x commits when having 6.x commits #28188

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep commits and translog up to the global checkpoint #27606

Keep commits and translog up to the global checkpoint #27606

dnhatn commented Nov 30, 2017

DaveCTurner left a comment

DaveCTurner Dec 1, 2017

DaveCTurner Dec 1, 2017

DaveCTurner Dec 1, 2017

dnhatn Dec 1, 2017

DaveCTurner Dec 1, 2017

dnhatn Dec 1, 2017

DaveCTurner Dec 1, 2017

dnhatn Dec 1, 2017

DaveCTurner Dec 1, 2017

dnhatn Dec 1, 2017

DaveCTurner Dec 1, 2017

dnhatn Dec 1, 2017

bleskes Dec 5, 2017

ywelsch Dec 1, 2017

dnhatn Dec 1, 2017

dnhatn Dec 1, 2017

bleskes commented Dec 1, 2017

dnhatn commented Dec 1, 2017 •

edited

Loading

dnhatn commented Dec 3, 2017

dnhatn commented Dec 3, 2017

bleskes commented Dec 3, 2017

DaveCTurner commented Dec 4, 2017

bleskes left a comment

bleskes Dec 4, 2017

dnhatn Dec 4, 2017 •

edited

Loading

dnhatn commented Dec 4, 2017

bleskes commented Dec 4, 2017

dnhatn commented Dec 12, 2017

bleskes left a comment

bleskes Dec 12, 2017

dnhatn commented Dec 12, 2017

DaveCTurner left a comment

dnhatn commented Dec 13, 2017

Keep commits and translog up to the global checkpoint #27606

Keep commits and translog up to the global checkpoint #27606

Conversation

dnhatn commented Nov 30, 2017

DaveCTurner 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes commented Dec 1, 2017

dnhatn commented Dec 1, 2017 • edited Loading

dnhatn commented Dec 3, 2017

dnhatn commented Dec 3, 2017

bleskes commented Dec 3, 2017

DaveCTurner commented Dec 4, 2017

bleskes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn Dec 4, 2017 • edited Loading

Choose a reason for hiding this comment

dnhatn commented Dec 4, 2017

bleskes commented Dec 4, 2017

dnhatn commented Dec 12, 2017

bleskes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn commented Dec 12, 2017

DaveCTurner left a comment

Choose a reason for hiding this comment

dnhatn commented Dec 13, 2017

dnhatn commented Dec 1, 2017 •

edited

Loading

dnhatn Dec 4, 2017 •

edited

Loading