Fold EngineDiskUtils into Store, for better lock semantics #29156

bleskes · 2018-03-20T07:46:04Z

#28245 has introduced the utility classEngineDiskUtils with a set of methods to prepare/change translog and lucene commit points. That util class bundled everything that's needed to create and empty shard, bootstrap a shard from a lucene index that was just restored etc.

In order to safely do these manipulations, the util methods acquired the IndexWriter's lock. That would sometime fail due to concurrent shard store fetching or other short activities that require the files not to be changed while they read from them.

Since there is no way to wait on the index writer lock, the Store class has other locks to make sure that once we try to acquire the IW lock, it will succeed. To side step this waiting problem, this PR folds EngineDiskUtils into Store. Sadly this comes with a price - the store class doesn't and shouldn't know about the translog. As such the logic is slightly less tight and callers have to do the translog manipulations on their own.

elasticmachine · 2018-03-20T07:46:05Z

Pinging @elastic/es-distributed

s1monw

Left some minors Looks good @bleskes

s1monw · 2018-03-21T12:27:12Z

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

 /**
 * This class contains utility methods for mutating the shard lucene index and translog as a preparation to be opened.
 */
 public abstract class EngineDiskUtils {

-    /**


this class is empty now?

woops. will remove.

s1monw · 2018-03-21T12:30:21Z

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

+        return userData;
+    }
+
+    private IndexWriter newIndexWriter(final boolean create, final Directory dir) throws IOException {


just pass an open mode instead of the redundant create flag?

s1monw · 2018-03-21T12:30:54Z

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

+            if (translogUUID.equals(getUserData(writer).get(Translog.TRANSLOG_UUID_KEY))) {
+                throw new IllegalArgumentException("a new translog uuid can't be equal to existing one. got [" + translogUUID + "]");
+            }
+


extra newline?

s1monw · 2018-03-21T12:31:24Z

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

+    /**
+     * Force bakes the given translog generation as recovery information in the lucene index.
+     */
+    public void associateIndexWithNewTranslog(final String translogUUID)


should this override the existing one? what is the usecase for associating a new one? can you document it?

This is the "ugly" part of the PR. What we're doing is create an empty translog and bake it's new uuid into an existing lucene index. This is done during snapshot restore and file based peer recovery. Before it was bundled nicely in one method but now it has be done out of store and the uuid passed in here. I will add this to the java docs.

hmm seems dangerous, can we assert stuff here?

Agreed. I tried to come up with some check but couldn't. I'll give it some more thought tmr morning.

s1monw · 2018-03-21T12:32:26Z

docs/reference/indices/flush.asciidoc

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


hmm why did this change?

To create an empty shard, we now:

Create an empty index via Store.createEmpty

Create an empty translog

Associate it with the index using Store. associateIndexWithNewTranslog

This creates one more commit compared to how it used to be. I can change createEmpty to require a translogUUID as a parameter , if you prefer. I'm OK either way.

bleskes · 2018-03-21T13:38:10Z

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

+
+
+    /**
+     * Marks an existing lucene index and marks it with a new history uuid. This should be called on


This java docs is stale. I'll work on it.

…tore

dnhatn

LGTM.

dnhatn · 2018-03-22T19:20:09Z

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

+     * This is used to make sure no existing shard will recovery from this index using ops based recovery.
+     */
+    public void bootstrapNewHistory()
+        throws IOException {


nit: we may not a new line here.

dnhatn · 2018-03-22T19:21:12Z

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

+     * created and the existing lucene index needs should be changed to use it.
+     */
+    public void associateIndexWithNewTranslog(final String translogUUID)
+        throws IOException {


nit: same here - we may not need this new line.

…tore # Conflicts: # server/src/test/java/org/elasticsearch/index/engine/EngineDiskUtilsTests.java

s1monw

LGTM

bleskes · 2018-03-26T12:08:48Z

Thx @s1monw , @dnhatn

* master: (40 commits) Do not optimize append-only if seen normal op with higher seqno (elastic#28787) [test] packaging: gradle tasks for groovy tests (elastic#29046) Prune only gc deletes below local checkpoint (elastic#28790) remove testUnassignedShardAndEmptyNodesInRoutingTable elastic#28745: remove extra option in the composite rest tests Fold EngineDiskUtils into Store, for better lock semantics (elastic#29156) Add file permissions checks to precommit task Remove execute mode bit from source files Optimize the composite aggregation for match_all and range queries (elastic#28745) [Docs] Add rank_eval size parameter k (elastic#29218) [DOCS] Remove ignore_z_value parameter link Docs: Update docs/index_.asciidoc (elastic#29172) Docs: Link C++ client lib elasticlient (elastic#28949) [DOCS] Unregister repository instead of deleting it (elastic#29206) Docs: HighLevelRestClient#multiSearch (elastic#29144) Add Z value support to geo_shape Remove type casts in logging in server component (elastic#28807) Change BroadcastResponse from ToXContentFragment to ToXContentObject (elastic#28878) REST : Split `RestUpgradeAction` into two actions (elastic#29124) Add error file docs to important settings ...

* es/master: (22 commits) Fix building Javadoc JARs on JDK for client JARs (#29274) Require JDK 10 to build Elasticsearch (#29174) Decouple NamedXContentRegistry from ElasticsearchException (#29253) Docs: Update generating test coverage reports (#29255) [TEST] Fix issue with HttpInfo passed invalid parameter Remove all dependencies from XContentBuilder (#29225) Fix sporadic failure in CompositeValuesCollectorQueueTests Propagate ignore_unmapped to inner_hits (#29261) TEST: Increase timeout for testPrimaryReplicaResyncFailed REST client: hosts marked dead for the first time should not be immediately retried (#29230) TEST: Use different translog dir for a new engine Make SearchStats implement Writeable (#29258) [Docs] Spelling and grammar changes to reindex.asciidoc (#29232) Do not optimize append-only if seen normal op with higher seqno (#28787) [test] packaging: gradle tasks for groovy tests (#29046) Prune only gc deletes below local checkpoint (#28790) remove testUnassignedShardAndEmptyNodesInRoutingTable #28745: remove extra option in the composite rest tests Fold EngineDiskUtils into Store, for better lock semantics (#29156) Add file permissions checks to precommit task ...

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

#28245 has introduced the utility class`EngineDiskUtils` with a set of methods to prepare/change translog and lucene commit points. That util class bundled everything that's needed to create and empty shard, bootstrap a shard from a lucene index that was just restored etc. In order to safely do these manipulations, the util methods acquired the IndexWriter's lock. That would sometime fail due to concurrent shard store fetching or other short activities that require the files not to be changed while they read from them. Since there is no way to wait on the index writer lock, the `Store` class has other locks to make sure that once we try to acquire the IW lock, it will succeed. To side step this waiting problem, this PR folds `EngineDiskUtils` into `Store`. Sadly this comes with a price - the store class doesn't and shouldn't know about the translog. As such the logic is slightly less tight and callers have to do the translog manipulations on their own.

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

bleskes added 2 commits March 19, 2018 11:29

Fold EngineDiskUtils into store

61264de

translog path get out of there

5e6746b

bleskes added >enhancement :Distributed Indexing/Store Issues around managing unopened Lucene indices. If it touches Store.java, this is a likely label. v7.0.0 v6.3.0 :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. labels Mar 20, 2018

bleskes requested review from s1monw and dnhatn March 20, 2018 07:46

s1monw suggested changes Mar 21, 2018

View reviewed changes

bleskes commented Mar 21, 2018

View reviewed changes

bleskes added 2 commits March 22, 2018 08:44

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

0bcfa41

…tore

feedback

5ebca23

bleskes requested a review from s1monw March 22, 2018 08:30

dnhatn approved these changes Mar 22, 2018

View reviewed changes

bleskes added 2 commits March 26, 2018 11:07

new lines

f9294c3

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

a91fffd

…tore # Conflicts: # server/src/test/java/org/elasticsearch/index/engine/EngineDiskUtilsTests.java

s1monw approved these changes Mar 26, 2018

View reviewed changes

compile, my friend

fb56e9e

bleskes merged commit f5d4550 into elastic:master Mar 26, 2018

bleskes deleted the engine_utils_to_store branch March 26, 2018 12:08

bleskes added the backport pending label Mar 26, 2018

dnhatn removed the backport pending label Mar 31, 2018

jpountz removed the :Distributed Indexing/Engine Anything around managing Lucene and the Translog in an open shard. label Jan 29, 2019

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

Fold EngineDiskUtils into Store, for better lock semantics #29156

Fold EngineDiskUtils into Store, for better lock semantics #29156

bleskes commented Mar 20, 2018

elasticmachine commented Mar 20, 2018

s1monw left a comment

s1monw Mar 21, 2018

bleskes Mar 21, 2018

s1monw Mar 21, 2018

bleskes Mar 21, 2018

s1monw Mar 21, 2018

s1monw Mar 21, 2018

bleskes Mar 21, 2018

s1monw Mar 22, 2018

bleskes Mar 22, 2018

s1monw Mar 21, 2018

bleskes Mar 21, 2018 •

edited

Loading

bleskes Mar 21, 2018

dnhatn left a comment

dnhatn Mar 22, 2018

dnhatn Mar 22, 2018

s1monw left a comment

bleskes commented Mar 26, 2018



		/**
		* Marks an existing lucene index and marks it with a new history uuid. This should be called on

Fold EngineDiskUtils into Store, for better lock semantics #29156

Fold EngineDiskUtils into Store, for better lock semantics #29156

Conversation

bleskes commented Mar 20, 2018

elasticmachine commented Mar 20, 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

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 Mar 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnhatn left a 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

bleskes commented Mar 26, 2018

bleskes Mar 21, 2018 •

edited

Loading