Clean up commits when global checkpoint advanced by dnhatn · Pull Request #28140 · elastic/elasticsearch

dnhatn · 2018-01-08T22:34:36Z

Today we keep multiple index commits based on the global checkpoint, but only clean up old index commits when we have a new index commit. However, we can release unneeded index commits earlier once the global checkpoint has advanced enough. This commit makes an engine revisit the index deletion policy whenever a new global checkpoint value is persisted.

Relates #10708

Today we keep multiple index commits based on the global checkpoint, but only clean up old index commits when we have a new index commit. We however can release unneeded index commits earlier once the global checkpoint has advanced enough. This commit revisits the index deletion policy whenever a new global checkpoint value is persisted.

# Conflicts: # server/src/main/java/org/elasticsearch/index/engine/CombinedDeletionPolicy.java # server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java # server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

# Conflicts: # server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

bleskes

Thanks @dnhatn . I left some feedback.

bleskes · 2018-01-17T03:24:44Z

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

+    /**
+     * Checks if the deletion policy can release some index commits with the latest global checkpoint.
+     */
+    synchronized boolean hasUnreferencedCommits() throws IOException {


a couple of comments here:

does this needs to be synchronized? can't we use volatile for safeCommit?

can we also pre-empt the long parsing etc with a check whether safeCommit != lastCommit? without it this will keep on returning true when the safe commit is safe and is the only commit?

Do won't we want to check for the moment when the lastCommit becomes safe? I'm a bit confused by the current implementation. Under normal circumstances the global checkpoint is > max seq no for the safe commit?

@bleskes Very good catch. This implementation is incorrect. We should compare the global checkpoint to the max_seqno of a commit after the safe commit.

@dnhatn just to be clear, I don't think we should be heroic and start doing the "optimal" thing of checking the commit after the safe commit (which will mean starting to store more info). I think we can rely on just checking the last commit. It will become safe very quickly.

bleskes · 2018-01-17T03:25:38Z

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

+    /**
+     * This method should be called after the translog has been synced.
+     */
+    public void onTranslogSynced() throws IOException {


I think we should do this differently, rather than sync the translog directly, we can make the translog sync method package private and have all syncs go through the engine. wdyt?

bleskes · 2018-01-17T03:26:36Z

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

            try {
                final Engine engine = getEngine();
                engine.getTranslog().ensureSynced(candidates.stream().map(Tuple::v1));
+                engine.onTranslogSynced();


this only need to happened if ensuredSync returned true.

dnhatn · 2018-01-17T16:02:52Z

@bleskes I've updated the comparison and made translog-sync methods go through the engine. However, Translog and Engine are in different packages, I had to keep translog-sync methods public. I made sure that Engine is the only consumer. Please give it another look when you have time. Thank you!

bleskes

Looks great. Left some very minor comments

bleskes · 2018-01-17T20:51:00Z

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

+        revisitIndexDeletionPolicy();
+    }
+
+    private void revisitIndexDeletionPolicy() throws IOException {


revisitIndexDeletionPolicyOnTranslogSync?

bleskes · 2018-01-17T20:52:31Z

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

+        IndexCommit safeCommit = randomFrom(commitList);
+        globalCheckpoint.set(Long.parseLong(safeCommit.getUserData().get(SequenceNumbers.MAX_SEQ_NO)));
+        indexPolicy.onCommit(commitList);
+        globalCheckpoint.set(randomLongBetween(globalCheckpoint.get(), lastMaxSeqNo)); // Advanced not enough


why is this not enough? we use >= in our check?

bleskes · 2018-01-17T20:53:27Z

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

+        if (safeCommit == commitList.get(commitList.size() - 1)) {
+            assertThat(indexPolicy.hasUnreferencedCommits(), equalTo(false)); // Keeping a single commit
+        } else {
+            assertThat(indexPolicy.hasUnreferencedCommits(), equalTo(true));


can we run another on commit and check that we just have one commit left and that the indexPolicy. hasUnreferencedCommits now returns true?

bleskes · 2018-01-17T20:55:40Z

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

+            for (int docId = 0; docId < numDocs; docId++) {
+                index(engine, docId);
+                if (rarely()) {
+                    globalCheckpoint.set(randomLongBetween(globalCheckpoint.get(), engine.getLocalCheckpointTracker().getCheckpoint()));


don't we have to use engine.getLocalCheckpointTracker().getCheckpoint()-1 to make sure the rest goes well and the GCP doesn't go backwards?

This does not bring special value but makes test harder to read.

dnhatn · 2018-01-17T21:51:08Z

@bleskes Your comments are addressed. Please give it another go. Thank you.

dnhatn · 2018-01-17T22:19:08Z

please test this.

bleskes

LGTM. Thanks Nhat

bleskes · 2018-01-18T20:19:57Z

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

+    /**
+     * Checks if the deletion policy can release some index commits with the latest global checkpoint.
+     */
+    boolean hasUnreferencedCommits() throws IOException {


just an idea for a follow up, shall we extend this method to look at the fact that a snapshotted commit was released (via a special flag we set when a snapshot count reaches 0)? we need to figure where to call it, but I think might be worth exploring.

@bleskes, I am thinking to always revisit the index deletion policy (without checking the unreferenced condition) when releasing a snapshotted commit in an Engine (InternalEngine#acquireIndexCommit). We don't acquire index commits too frequently and revisiting the policy should not be expensive. WDYT?

@dnhatn I think I wasn't clear. The idea was to add a "pendingSnapshots" flag that is set when a snapshot reference goes to 0 and is cleared onCommit.

Thanks @bleskes. I got the idea.

dnhatn · 2018-01-18T20:44:40Z

Thanks @bleskes for your helpful reviews.

* es/master: (38 commits) Build: Add pom generation to meta plugins (#28321) Add 6.3 version constant to master Minor improvements to translog docs (#28237) [Docs] Remove typo in painless-getting-started.asciidoc Build: Fix meta plugin usage in integ test clusters (#28307) Painless: Add spi jar that will be published for extending whitelists (#28302) mistyping in one of the highlighting examples comment -> content (#28139) Documents applicability of term query to range type (#28166) Build: Omit dependency licenses check for elasticsearch deps (#28304) Clean up commits when global checkpoint advanced (#28140) Implement socket and server ChannelContexts (#28275) Plugins: Fix meta plugins to install bundled plugins with their real name (#28285) Build: Fix meta plugin integ test installation (#28286) Modify Abstract transport tests to use impls (#28270) Fork Groovy compiler onto compile Java home [Docs] Update tophits-aggregation.asciidoc (#28273) Docs: match between snippet to its description (#28296) [TEST] fix RequestTests#testSearch in case search source is not set REST high-level client: remove index suffix from indices client method names (#28263) Fix simple_query_string on invalid input (#28219) ...

Today we keep multiple index commits based on the current global checkpoint, but only clean up unneeded index commits when we have a new index commit. However, we can release the old index commits earlier once the global checkpoint has advanced enough. This commit makes an engine revisit the index deletion policy whenever a new global checkpoint value is persisted and advanced enough. Relates #10708

* 6.x: Trim down usages of `ShardOperationFailedException` interface (#28312) Clean up commits when global checkpoint advanced (#28140) Do not return all indices if a specific alias is requested via get aliases api. CountedBitSet doesn't need to extend BitSet. (#28239) Calculate sum in Kahan summation algorithm in aggregations (#27807) (#27848)

A follow-up of elastic#28140 We currently revisit the index deletion policy whenever the global checkpoint has advanced enough. However, we won't be able to clean up the old commit points if they are being snapshotted. Here we prefer a simple solution over an optimal solution as we should revisit if only the last commit is being snapshotted.

We currently revisit the index deletion policy whenever the global checkpoint has advanced enough. We should also revisit the deletion policy after releasing the last snapshot of a snapshotting commit. With this change, the old index commits will be cleaned up as soon as possible. Follow-up of #28140 #28140 (comment)

Since elastic#28140 when the global checkpoint is advanced, we try to move the safe commit forward, and clean old index commits if possible. However, we forget to trim unreferenced translog. This change makes sure that we prune both translog and index commits when the safe commit advanced. Relates elastic#28140 Closes elastic#32089

Since #28140 when the global checkpoint is advanced, we try to move the safe commit forward, and clean up old index commits if possible. However, we forget to trim unreferenced translog. This change makes sure that we prune both old translog and index commits when the safe commit advanced. Relates #28140 Closes #32089

dnhatn added >enhancement review v7.0.0 v6.2.0 labels Jan 8, 2018

dnhatn requested review from bleskes, jasontedor and ywelsch January 8, 2018 22:34

dnhatn added 2 commits January 11, 2018 22:05

Do not cache the max_seqno from the last commit

0cacc44

jasontedor removed their request for review January 12, 2018 15:51

dnhatn added 4 commits January 13, 2018 22:48

Merge branch 'master' into clean-commit-when-gcp-advanced

8ddffa8

Merge branch 'master' into clean-commit-when-gcp-advanced

80c415f

# Conflicts: # server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

Fix async global checkpoint test

fa8f3fa

Merge branch 'master' into clean-commit-when-gcp-advanced

315162b

bleskes suggested changes Jan 17, 2018

View reviewed changes

dnhatn added 2 commits January 17, 2018 10:54

Checks againts the last commit

284e7f3

Sync translog from outside

946c09f

bleskes suggested changes Jan 17, 2018

View reviewed changes

dnhatn added 2 commits January 17, 2018 16:40

Feedbacks

7466647

No need to sync gcp frequently

2b3cf38

This does not bring special value but makes test harder to read.

bleskes approved these changes Jan 18, 2018

View reviewed changes

dnhatn merged commit 9db9bd5 into elastic:master Jan 18, 2018

dnhatn added the backport pending label Jan 18, 2018

bleskes mentioned this pull request Jan 18, 2018

Add Sequence Numbers to write operations #10708

Closed

64 tasks

dnhatn deleted the clean-commit-when-gcp-advanced branch January 18, 2018 20:46

dnhatn added v6.3.0 and removed v6.2.0 labels Jan 18, 2018

dnhatn removed the backport pending label Jan 22, 2018

dnhatn mentioned this pull request Feb 12, 2018

Revisit deletion policy after release the last snapshot #28627

Merged

clintongormley added :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. and removed :Sequence IDs labels Feb 14, 2018

dnhatn mentioned this pull request Aug 17, 2018

Trim unreferenced translog when the safe commit advanced #32967

Merged

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

Conversation

dnhatn commented Jan 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bleskes 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!

dnhatn commented Jan 17, 2018

Uh oh!

bleskes 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!

dnhatn commented Jan 17, 2018

Uh oh!

dnhatn commented Jan 17, 2018

Uh oh!

bleskes 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!

dnhatn commented Jan 18, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dnhatn commented Jan 8, 2018 •

edited

Loading