Rebuild version map when opening internal engine #43202

dnhatn · 2019-06-13T14:41:50Z

With this change, we will rebuild the live version map and local checkpoint using documents (including soft-deleted) from the safe commit when opening an internal engine. This allows us to safely prune away _id of all soft-deleted documents as the version map is always in-sync with Lucene index.

Relates #40741
Supersedes #42979

To minimize this PR, I will work on two follow-ups:

Adjust testLookupSeqNoByIdInLucene
Remove MapperService parameter from readHistoryOperations estimateNumberOfHistoryOperations since we have it in EngineConfig now.

elasticmachine · 2019-06-13T14:41:54Z

Pinging @elastic/es-distributed

dnhatn · 2019-06-13T14:59:11Z

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

@@ -5856,4 +5888,37 @@ public void testPruneAwayDeletedButRetainedIds() throws Exception {
            }
        }
    }
+
+    public void testRecoverFromLocalTranslog() throws Exception {


This test fails on master and 7.x (1 of 5000) without this change.

cool can we try to make a test that fails more often?

The test fails when we have out of order in the safe commit and a merge prunes _id. This test index/delete/flush/merge randomly then restarting an engine should yield the same documents as before. I can make it fail more happen but I prefer to leave as it is (hope we can catch other situations). Are you okay?

are there other tests that are failing more often than 1 in 5000, to make sure that this change has enough coverage?

Yes, I can add that test in a follow-up.

s1monw

nice change I left some comments

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

s1monw · 2019-06-13T19:08:00Z

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

            this.lastRefreshedCheckpointListener = new LastRefreshedCheckpointListener(localCheckpointTracker.getCheckpoint());
            this.internalSearcherManager.addListener(lastRefreshedCheckpointListener);
            maxSeqNoOfUpdatesOrDeletes = new AtomicLong(SequenceNumbers.max(localCheckpointTracker.getMaxSeqNo(), translog.getMaxSeqNo()));
+            if (softDeleteEnabled && localCheckpointTracker.getCheckpoint() < localCheckpointTracker.getMaxSeqNo()) {
+                try (Searcher searcher = acquireSearcher("restore_version_map_and_checkpoint_tracker", SearcherScope.INTERNAL)) {
+                    restoreVersionMapAndCheckpointTracker(Lucene.wrapAllDocsLive(searcher.getDirectoryReader()));


can we actually call flush after that to make sure we commit and clear the version map again?

can we actually skip this entire dance if we don't have any deletes in the index? just curious...

can we actually skip this entire dance if we don't have any deletes in the index? just curious...

Yes, we can. If there's no delete, the version map will empty after this method. For the local checkpoint, I think we would like to have it in sync Lucene index. Yannick, WDYT?

can we actually skip this entire dance if we don't have any deletes in the index? just curious...

I think we could even skip it if there are no soft-deletes in the range we're looking for (i.e. from local checkpoint of safe commit upwards). I would like to avoid special-casing this too much though, and think we should just always do the full thing here. This should be very fast, given that this is a subset of the range that we will have to replay from the translog, which will certainly take much longer.

s1monw · 2019-06-13T19:09:48Z

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

@@ -5856,4 +5888,37 @@ public void testPruneAwayDeletedButRetainedIds() throws Exception {
            }
        }
    }
+
+    public void testRecoverFromLocalTranslog() throws Exception {


cool can we try to make a test that fails more often?

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

ywelsch

Looks great. I've left some comments. Did not look at the tests yet.

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

dnhatn · 2019-06-14T03:32:26Z

@s1monw @ywelsch Thanks for your input. This is ready for another round.

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

ywelsch · 2019-06-14T11:02:46Z

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

+            }
+        }
+        // remove live entries in the version map
+        refresh("restore_version_map_and_checkpoint_tracker", SearcherScope.INTERNAL, true);


Isn't this just calling maybePruneDeletes, which seems to only run if X amount of time has passed after the last pruning? Isn't this a problem in general if we want to mainly do pruning based on local checkpoint?

We can manually call versionMap.pruneTombstones(Long.MAX_VALUE, getLocalCheckpoint()); after the refresh. However, I expect it won't prune anything since all delete tombstones should be above the local checkpoint.

The local checkpoint in the commit is a conservative approximation of the the true local checkpoint, but you're right, this should not lead to much pruning here. I'm generally concerned with the maybePruneDeletes method, which only seems to care about timestamps, and not local checkpoint advancement. Not something we need to solve in this PR, just a more general observation.

ywelsch · 2019-06-14T11:15:50Z

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

            this.lastRefreshedCheckpointListener = new LastRefreshedCheckpointListener(localCheckpointTracker.getCheckpoint());
            this.internalSearcherManager.addListener(lastRefreshedCheckpointListener);
            maxSeqNoOfUpdatesOrDeletes = new AtomicLong(SequenceNumbers.max(localCheckpointTracker.getMaxSeqNo(), translog.getMaxSeqNo()));
+            if (softDeleteEnabled && localCheckpointTracker.getCheckpoint() < localCheckpointTracker.getMaxSeqNo()) {
+                try (Searcher searcher = acquireSearcher("restore_version_map_and_checkpoint_tracker", SearcherScope.INTERNAL)) {
+                    restoreVersionMapAndCheckpointTracker(Lucene.wrapAllDocsLive(searcher.getDirectoryReader()));


can we actually skip this entire dance if we don't have any deletes in the index? just curious...

I think we could even skip it if there are no soft-deletes in the range we're looking for (i.e. from local checkpoint of safe commit upwards). I would like to avoid special-casing this too much though, and think we should just always do the full thing here. This should be very fast, given that this is a subset of the range that we will have to replay from the translog, which will certainly take much longer.

dnhatn · 2019-06-14T13:37:38Z

@s1monw @ywelsch I pushed changes. Can you please take another look? Thank you!

s1monw

LGTM

ywelsch

LGTM

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

ywelsch · 2019-06-17T07:23:12Z

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

+            }
+        }
+        // remove live entries in the version map
+        refresh("restore_version_map_and_checkpoint_tracker", SearcherScope.INTERNAL, true);


The local checkpoint in the commit is a conservative approximation of the the true local checkpoint, but you're right, this should not lead to much pruning here. I'm generally concerned with the maybePruneDeletes method, which only seems to care about timestamps, and not local checkpoint advancement. Not something we need to solve in this PR, just a more general observation.

server/src/main/java/org/elasticsearch/index/fieldvisitor/IdOnlyFieldVisitor.java

ywelsch · 2019-06-17T07:30:17Z

server/src/main/java/org/elasticsearch/index/fieldvisitor/IdOnlyFieldVisitor.java

+    }
+
+    public String getId() {
+        return id;


should this throw an exception if visited == false?

visited can be false for Noops which do not have _id.

ywelsch · 2019-06-17T07:41:41Z

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

@@ -5856,4 +5888,37 @@ public void testPruneAwayDeletedButRetainedIds() throws Exception {
            }
        }
    }
+
+    public void testRecoverFromLocalTranslog() throws Exception {


are there other tests that are failing more often than 1 in 5000, to make sure that this change has enough coverage?

henningandersen

LGTM.

I left one optional comment.

henningandersen · 2019-06-17T10:29:37Z

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

+            }
+        }
+        // remove live entries in the version map
+        refresh("restore_version_map_and_checkpoint_tracker", SearcherScope.INTERNAL, true);


Given that this runs in the constructor, I wonder if it was better to simply clear the live version maps rather than call refresh here? Calling refresh calls listeners and we thus call those while the engine is half constructed (especially when the actual engine is the FollowingEngine). I found no cases where this would be problematic currently, so will leave it up to you.

Good point. I have prototyped a change for this but I think it requires a full review. I will work on a follow-up.

dnhatn · 2019-06-17T21:50:27Z

@s1monw @ywelsch @henningandersen Thanks a lot for your great feedback and proposing this idea.

With this change, we will rebuild the live version map and local checkpoint using documents (including soft-deleted) from the safe commit when opening an internal engine. This allows us to safely prune away _id of all soft-deleted documents as the version map is always in-sync with the Lucene index. Relates #40741 Supersedes #42979

This PR reverts #35230. Previously, we reply on soft-deletes to fill the mismatch between the version map and the Lucene index. This is no longer needed after #43202 where we rebuild the version map when opening an engine. Moreover, PrunePostingsMergePolicy can prune _id of soft-deleted documents out of order; thus the lookup result including soft-deletes sometimes does not return the latest version (although it's okay as we only use a valid result in an engine). With this change, we use only live documents in Lucene to resolve the indexing strategy. This is perfectly safe since we keep all deleted documents after the local checkpoint in the version map. Closes #42979

With this change, we will rebuild the live version map and local checkpoint using documents (including soft-deleted) from the safe commit when opening an internal engine. This allows us to safely prune away _id of all soft-deleted documents as the version map is always in-sync with Lucene index. Relates #40741 (elastic/elasticsearch#40741) Supersedes #42979 (elastic/elasticsearch#42979) Port of elastic/elasticsearch#43202

With this change, we will rebuild the live version map and local checkpoint using documents (including soft-deleted) from the safe commit when opening an internal engine. This allows us to safely prune away _id of all soft-deleted documents as the version map is always in-sync with Lucene index. Relates #40741 (elastic/elasticsearch#40741) Supersedes #42979 (elastic/elasticsearch#42979) Port of elastic/elasticsearch#43202 (cherry picked from commit bb23cb7) # Conflicts: # es/es-server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java # es/es-server/src/test/java/org/elasticsearch/index/engine/InternalEngineTests.java

With this change, we will rebuild the live version map and local checkpoint using documents (including soft-deleted) from the safe commit when opening an internal engine. This allows us to safely prune away _id of all soft-deleted documents as the version map is always in-sync with Lucene index. Relates #40741 (elastic/elasticsearch#40741) Supersedes #42979 (elastic/elasticsearch#42979) Port of elastic/elasticsearch#43202 (cherry picked from commit bb23cb7)

Rebuild version map when opening internal engine

42c8233

dnhatn added >enhancement :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. v8.0.0 v7.3.0 labels Jun 13, 2019

dnhatn requested review from jpountz, s1monw, ywelsch and henningandersen June 13, 2019 14:41

dnhatn mentioned this pull request Jun 13, 2019

Prune _id of only docs below local checkpoint of safe commit #43051

Closed

dnhatn commented Jun 13, 2019

View reviewed changes

s1monw suggested changes Jun 13, 2019

View reviewed changes

Merge branch 'master' into rebuild-version-map

4a4fe69

ywelsch suggested changes Jun 13, 2019

View reviewed changes

dnhatn added 2 commits June 13, 2019 17:08

simon’s feedback

711b00c

yannick’s feedback

14b6ebf

dnhatn requested review from s1monw and ywelsch June 14, 2019 03:32

s1monw reviewed Jun 14, 2019

View reviewed changes

server/src/main/java/org/elasticsearch/index/engine/InternalEngine.java Outdated Show resolved Hide resolved

ywelsch suggested changes Jun 14, 2019

View reviewed changes

dnhatn added 4 commits June 14, 2019 09:09

Merge branch 'master' into rebuild-version-map

b6c29bc

make it static

e67fa1e

use 0L so we can prune quickly

f9d6229

Reuse field visitor

a7b2bea

dnhatn requested review from s1monw and ywelsch June 14, 2019 13:37

s1monw approved these changes Jun 15, 2019

View reviewed changes

ywelsch approved these changes Jun 17, 2019

View reviewed changes

henningandersen approved these changes Jun 17, 2019

View reviewed changes

dnhatn added 3 commits June 17, 2019 12:05

Merge branch 'master' into rebuild-version-map

1f1e8e5

wording

a24c25a

assertion first

1cb6436

dnhatn merged commit bbc29bb into elastic:master Jun 17, 2019

dnhatn deleted the rebuild-version-map branch June 17, 2019 21:50

dnhatn added the backport pending label Jun 17, 2019

dnhatn removed the backport pending label Jun 17, 2019

dnhatn mentioned this pull request Jun 18, 2019

Do not use soft-deletes to resolve indexing strategy #43336

Merged

marregui mentioned this pull request Nov 28, 2019

Rebuild version map when opening internal engine crate/crate#9394

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebuild version map when opening internal engine #43202

Rebuild version map when opening internal engine #43202

dnhatn commented Jun 13, 2019

elasticmachine commented Jun 13, 2019

dnhatn Jun 13, 2019 •

edited

Loading

s1monw Jun 13, 2019

dnhatn Jun 14, 2019

ywelsch Jun 17, 2019

dnhatn Jun 17, 2019

s1monw left a comment

s1monw Jun 13, 2019

s1monw Jun 13, 2019

dnhatn Jun 14, 2019

ywelsch Jun 14, 2019

s1monw Jun 13, 2019

ywelsch left a comment

dnhatn commented Jun 14, 2019

ywelsch Jun 14, 2019

dnhatn Jun 14, 2019 •

edited

Loading

ywelsch Jun 17, 2019

ywelsch Jun 14, 2019

dnhatn commented Jun 14, 2019

s1monw left a comment

ywelsch left a comment

ywelsch Jun 17, 2019

ywelsch Jun 17, 2019

dnhatn Jun 17, 2019

ywelsch Jun 17, 2019

henningandersen left a comment

henningandersen Jun 17, 2019

dnhatn Jun 17, 2019

dnhatn commented Jun 17, 2019

Rebuild version map when opening internal engine #43202

Rebuild version map when opening internal engine #43202

Conversation

dnhatn commented Jun 13, 2019

elasticmachine commented Jun 13, 2019

dnhatn Jun 13, 2019 • edited Loading

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

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

ywelsch left a comment

Choose a reason for hiding this comment

dnhatn commented Jun 14, 2019

Choose a reason for hiding this comment

dnhatn Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn commented Jun 14, 2019

s1monw left a comment

Choose a reason for hiding this comment

ywelsch 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

henningandersen left a 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 17, 2019

dnhatn Jun 13, 2019 •

edited

Loading

dnhatn Jun 14, 2019 •

edited

Loading