Move trimming unsafe commits from the Engine constructor to Store #29260

bleskes · 2018-03-27T10:46:15Z

As follow up to #28245 , this PR removes the logic for selecting the right start commit from the Engine constructor in favor of explicitly trimming them in the Store, before the engine is opened. This makes the constructor in engine follow standard Lucene semantics and use the last commit.

# Conflicts: # docs/reference/indices/flush.asciidoc # server/src/main/java/org/elasticsearch/index/engine/EngineDiskUtils.java # server/src/main/java/org/elasticsearch/index/shard/IndexShard.java # server/src/test/java/org/elasticsearch/index/engine/EngineDiskUtilsTests.java

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

elasticmachine · 2018-03-27T10:46:51Z

Pinging @elastic/es-distributed

bleskes · 2018-03-27T10:48:27Z

docs/reference/indices/flush.asciidoc

@@ -93,12 +93,12 @@ which returns something similar to:
               {
                 "commit" : {
                   "id" : "3M3zkw2GHMo2Y4h4/KFKCg==",
-                   "generation" : 4,


This one was tricky to trace - using a starting commit Lucene, immediately increments the pending changes counter, which means that the flush issued in the docs will actually cause a commit in Lucene. Now that we don't use a starting commit, the uncommittedChanges counter in Lucene stays 0 when opening the engine and the flush in the docs becomes a noop.

s1monw

LGTM I think it's good to make as many decisions as possible before we create the engine!

s1monw · 2018-03-28T12:08:30Z

server/src/main/java/org/elasticsearch/index/store/Store.java

+                    // note that we can't just use IndexCommit.delete() as we really want to make sure that those files won't be used
+                    // even if a virus scanner causes the files not to be used.
+
+                    // TODO: speak to @s1monw about the fact that we can' use  getUserData(writer) as that uses that last's commit user


if you open an IW based on a commit it will use that commits user data if you use getUserData(writer) are you sure that is not the case. I just looked at the code and it looks correct

This is correct. Please see https://github.com/apache/lucene-solr/blob/d4e69c5cd868d0f5b71da0f4b23c2cd61d1b0ea0/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java#L1092

@s1monw
The new commit will use segment files from the starting commit but userData from the last commit by default. Therefore we need to manually set the userData from the starting commit to the new commit.

s1monw · 2018-03-28T12:08:53Z

server/src/main/java/org/elasticsearch/index/store/Store.java

+        metadataLock.writeLock().lock();
+        try {
+            final List<IndexCommit> existingCommits = DirectoryReader.listCommits(directory);
+            if (existingCommits.size() == 0) {


nit: use isEmpty()?

s1monw · 2018-03-28T12:09:06Z

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

@@ -1329,6 +1329,9 @@ private void innerOpenEngineAndTranslog() throws IOException {

        assertMaxUnsafeAutoIdInCommit();

+        final long minRetainedTranslogGen = Translog.readMinTranslogGeneration(translogConfig.getTranslogPath(), translogUUID);
+        store.trimUnsafeCommits(globalCheckpoint, minRetainedTranslogGen, config.getIndexSettings().getIndexVersionCreated());


dnhatn

LGTM.

dnhatn · 2018-03-28T13:49:33Z

server/src/main/java/org/elasticsearch/index/store/Store.java

+                    // note that we can't just use IndexCommit.delete() as we really want to make sure that those files won't be used
+                    // even if a virus scanner causes the files not to be used.
+
+                    // TODO: speak to @s1monw about the fact that we can' use  getUserData(writer) as that uses that last's commit user


This is correct. Please see https://github.com/apache/lucene-solr/blob/d4e69c5cd868d0f5b71da0f4b23c2cd61d1b0ea0/lucene/core/src/java/org/apache/lucene/index/IndexWriter.java#L1092

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

dnhatn · 2018-03-29T17:37:57Z

I am merging this on behalf of Boaz. Thanks @bleskes and @s1monw.

Since #29260, unsafe commits must be trimmed before opening an engine. This makes the engine constructor follow Lucene standard semantics and use the last commit. However, we haven't fully applied this change in some tests. Relates #29260

As follow up to #28245 , this PR removes the logic for selecting the right start commit from the Engine constructor in favor of explicitly trimming them in the Store, before the engine is opened. This makes the constructor in engine follow standard Lucene semantics and use the last commit. Relates #28245 Relates #29156

* master: (80 commits) Remove HTTP max content length leniency (elastic#29337) Begin moving XContent to a separate lib/artifact (elastic#29300) Java versions for ci (elastic#29320) Minor cleanup in the InternalEngine (elastic#29241) Clarify expectations of false positives/negatives (elastic#27964) Update docs on vertex ordering (elastic#27963) Revert "REST high-level client: add support for Indices Update Settings API (elastic#28892)" (elastic#29323) [test] remove Streamable serde assertions (elastic#29307) Improve query string docs (elastic#28882) fix query string example for boolean query (elastic#28881) Resolve unchecked cast warnings introduced with elastic#28892 REST high-level client: add support for Indices Update Settings API (elastic#28892) Search: Validate script query is run with a single script (elastic#29304) [DOCS] Added info on WGS-84. Closes issue elastic#3590 (elastic#29305) Increase timeout on Netty client latch for tests Build: Use branch specific refspec sysprop for bwc builds (elastic#29299) TEST: trim unsafe commits before opening engine Move trimming unsafe commits from engine ctor to store (elastic#29260) Fix incorrect geohash for lat 90, lon 180 (elastic#29256) Do not load global state when deleting a snapshot (elastic#29278) ...

bleskes added 7 commits March 18, 2018 17:20

no starting commit for you

7319310

doc change

0850ae3

add logc to store

3fbce14

back to 3

ca26f99

Merge remote-tracking branch 'upstream/master' into engine_utils_trim…

dbb8d25

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

remove new line

a179113

bleskes requested review from s1monw and dnhatn March 27, 2018 10:46

bleskes added >enhancement v7.0.0 v6.3.0 :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. labels Mar 27, 2018

bleskes commented Mar 27, 2018

View reviewed changes

s1monw approved these changes Mar 28, 2018

View reviewed changes

dnhatn approved these changes Mar 28, 2018

View reviewed changes

dnhatn self-assigned this Mar 28, 2018

dnhatn added 4 commits March 29, 2018 10:40

add comment why manual assign user data

491fb12

Use empty method

8447be1

Merge branch 'master' into pr/29260

d92f404

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

Remove unused imports

cccd663

dnhatn merged commit eb8b317 into elastic:master Mar 29, 2018

dnhatn added the backport pending label Mar 29, 2018

dnhatn removed the backport pending label Mar 31, 2018

colings86 added the v7.0.0-beta1 label Feb 7, 2019

colings86 removed the v7.0.0 label Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move trimming unsafe commits from the Engine constructor to Store #29260

Move trimming unsafe commits from the Engine constructor to Store #29260

bleskes commented Mar 27, 2018

elasticmachine commented Mar 27, 2018

bleskes Mar 27, 2018

s1monw left a comment

s1monw Mar 28, 2018

dnhatn Mar 28, 2018

dnhatn Mar 29, 2018

s1monw Mar 28, 2018

s1monw Mar 28, 2018

dnhatn left a comment

dnhatn Mar 28, 2018

dnhatn commented Mar 29, 2018

Move trimming unsafe commits from the Engine constructor to Store #29260

Move trimming unsafe commits from the Engine constructor to Store #29260

Conversation

bleskes commented Mar 27, 2018

elasticmachine commented Mar 27, 2018

bleskes Mar 27, 2018

Choose a reason for hiding this comment

s1monw left a comment

Choose a reason for hiding this comment

s1monw Mar 28, 2018

Choose a reason for hiding this comment

dnhatn Mar 28, 2018

Choose a reason for hiding this comment

dnhatn Mar 29, 2018

Choose a reason for hiding this comment

s1monw Mar 28, 2018

Choose a reason for hiding this comment

s1monw Mar 28, 2018

Choose a reason for hiding this comment

dnhatn left a comment

Choose a reason for hiding this comment

dnhatn Mar 28, 2018

Choose a reason for hiding this comment

dnhatn commented Mar 29, 2018