Untangle Engine Constructor logic #28245

bleskes · 2018-01-16T15:09:55Z

Currently we have a fairly complicated logic in the engine and translog's constructors logic to deal with all the various ways we want to mutate the lucene index and translog we're opening.

We can:

Create an empty index
Use the lucene but create a new translog
Use both
Force a new history uuid in all cases.

This leads complicated code flows which makes it harder and harder to make sure we cover all the corner cases. This PR tries to take another approach. Constructing an InternalEngine always opens things as they are and all needed modifications are done by static methods directly on the directory, one at a time.

The PR is still WIP (mostly around documentation) to get initial feedback about what people want think about it. If we like it I can break it up into a translog related PR and an a follow up for the engine.

…ning

…recovery types.

…ning

s1monw · 2018-01-17T08:54:48Z

the approach makes a lot of sense to me.

dnhatn

Looks great to me. I left some minor comments and a reminder.

dnhatn · 2018-01-18T14:45:08Z

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

-                config().getIndexSettings().getIndexVersionCreated().before(Version.V_6_0_0_rc1) :
-                "existing index was created after 6_0_0_rc1 but has no history uuid";
-            uuid = UUIDs.randomBase64UUID();
+    private String loadOrGenerateHistoryUUID(final IndexWriter writer) throws IOException {


The method name is no longer correct as we now only load the history uuid.

dnhatn · 2018-01-18T14:48:02Z

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

+    }
+
+
+    public static void verifyHasHistoryUUID(final Directory dir) throws IOException {


I though this is an assertion but not. Should we give it a better name?

dnhatn · 2018-01-18T14:49:18Z

server/src/main/java/org/elasticsearch/index/translog/Translog.java

@@ -131,23 +133,19 @@
     * translog file referenced by this generation. The translog creation will fail if this generation can't be opened.
     *
     * @param config                   the configuration of this translog
-     * @param expectedTranslogUUID     the translog uuid to open, null for a new translog
+     * @param translogUUID     the translog uuid to open, null for a new translog


nit: reformat javadoc

dnhatn · 2018-01-18T15:39:19Z

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

@@ -181,22 +177,14 @@ public InternalEngine(EngineConfig engineConfig) {
                assert translog.getGeneration() != null;
                this.translog = translog;
                final IndexCommit startingCommit = getStartingCommitPoint();
-                assert openMode != EngineConfig.OpenMode.OPEN_INDEX_AND_TRANSLOG || startingCommit != null :
-                    "Starting commit should be non-null; mode [" + openMode + "]; startingCommit [" + startingCommit + "]";
+                assert startingCommit != null : "Starting commit should be non-null";


Just to remind you that we discussed whether to move startingCommitPoint to EngineDiskUtils.

I decided to do this as a follow up.

…ning

bleskes · 2018-03-12T11:05:44Z

@dnhatn @s1monw I updated this PR and removed the POC label of it. Can you take another look? I think it's ready for full reviews. Thanks!

dnhatn

LGTM.

s1monw

this is a great change! LGTM

s1monw · 2018-03-13T15:33:17Z

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

+import java.util.List;
+import java.util.Map;
+
+


this needs javadocs

s1monw · 2018-03-13T15:34:29Z

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

+
+public final class EngineDiskUtils {
+
+    private EngineDiskUtils() {


as an alternative you can mark it as final abstract then you don't need the private ctor

I don't care. I've seen requests to go both ways. I just want people to be happy. I'll go with abstract. I

s1monw · 2018-03-13T15:35:54Z

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

+        final String translogUUID = store.readLastCommittedSegmentsInfo().getUserData().get(Translog.TRANSLOG_UUID_KEY);
+        final long globalCheckpoint = Translog.readGlobalCheckpoint(translogConfig.getTranslogPath(), translogUUID);
+        replicationTracker.updateGlobalCheckpointOnReplica(globalCheckpoint, "read from translog checkpoint");
+


what are these newlines for here?

they seperate the flow into units that are related. Something like paragraphs. If you don't like them I'll remove.

…ning # Conflicts: # test/framework/src/main/java/org/elasticsearch/index/engine/EngineTestCase.java

bleskes · 2018-03-14T20:00:26Z

Thx @s1monw & @dnhatn

bleskes · 2018-03-15T13:30:38Z

While backporting this to 6.3 I found a flaw in the logic in EngineDiskUtils methods - even if the don't do anything they trigger lucenes deletion policy which messes up with our safe commits. I can disable deletion in these methods but this points a deeper flaw. Since it commits the writer, it also expects the commit it wrote to be used as a starting commit - otherwise it's pointless. For example, it it wants to add a history uuid, it will create a new commit, based on the last one, with the added uuid. We have no guarantee at that point that that commit is actually safe.

Instead of tweaking these methods, I rather push forward with moving the "remove all unsafe commits" logic to this class. Then I can make sure that the current method only use "safe" commits as a base line.

#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

Currently we have a fairly complicated logic in the engine constructor logic to deal with all the various ways we want to mutate the lucene index and translog we're opening. We can: 1) Create an empty index 2) Use the lucene but create a new translog 3) Use both 4) Force a new history uuid in all cases. This leads complicated code flows which makes it harder and harder to make sure we cover all the corner cases. This PR tries to take another approach. Constructing an InternalEngine always opens things as they are and all needed modifications are done by static methods directly on the directory, one at a time.

#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

This commit adds missing logic during backporting the untangle engine constructor PR (#28245). These minor logic helps to handle an old indices which do not have all 6.x commit tags such as max_unsafe_autoid_timestamp or historyUUID or sequence numbers.

bleskes added 17 commits January 13, 2018 22:31

basic utils in place

2826b41

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

922bdb4

…ning

integrated

d9c236b

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

e1ed1dc

…ning

lint

5be23dc

fix index shard assertion

88bd738

fix translog tests

7816210

fix recovery tests

e13f0f2

remove assertion openEngineAndRecoverFromTranslog is now used by all …

963e86e

…recovery types.

fix RefreshListenersTests

aba67a4

fix testShardActiveDuringPeerRecovery

8d1fe79

fix testStressMaybeFlushOrRollTranslogGeneration

4bc7f9b

fix testMaybeFlush

d6d3f6c

properly acquire store reference in RecoveryTarget#cleanFiles

0bbd50b

fix translog retention rest tests

152aca3

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

f52ffd6

…ning

fix flush docs

2c36b88

bleskes added :Engine labels Jan 16, 2018

bleskes requested review from s1monw, ywelsch and dnhatn January 16, 2018 15:09

dnhatn reviewed Jan 18, 2018

View reviewed changes

bleskes added 6 commits January 20, 2018 19:00

merge from master

e78d93d

fix static method and compilation

5b9b8cb

missing sync

ac96ede

fix InternalEngineTests

3b11801

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

0b942b0

…ning

Merge branch 'master' into engine_simple_opening

8fa3106

lint

37853c9

bleskes changed the title ~~[POC] Untangle Engine & Translog Constructor logic~~ Untangle Engine Constructor logic Mar 12, 2018

bleskes requested review from dnhatn and s1monw and removed request for s1monw March 12, 2018 11:04

dnhatn approved these changes Mar 13, 2018

View reviewed changes

s1monw approved these changes Mar 13, 2018

View reviewed changes

bleskes added 2 commits March 14, 2018 13:23

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

72b9cb0

…ning # Conflicts: # test/framework/src/main/java/org/elasticsearch/index/engine/EngineTestCase.java

feedback

7139116

bleskes merged commit bf65cb4 into elastic:master Mar 14, 2018

bleskes deleted the engine_simple_opening branch March 14, 2018 20:00

bleskes added >enhancement v7.0.0 backport pending v6.3.0 labels Mar 15, 2018

bleskes mentioned this pull request Mar 20, 2018

Fold EngineDiskUtils into Store, for better lock semantics #29156

Merged

bleskes mentioned this pull request Mar 27, 2018

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

Merged

dnhatn removed the backport pending label Mar 31, 2018

praseodym mentioned this pull request Jun 20, 2018

5.6.10 to 6.3.0 rolling upgrade broken with 'commit doesn't contain history uuid' when a synced flush is performed #31482

Closed

jimczi 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

Untangle Engine Constructor logic #28245

Untangle Engine Constructor logic #28245

bleskes commented Jan 16, 2018

s1monw commented Jan 17, 2018

dnhatn left a comment

dnhatn Jan 18, 2018

dnhatn Jan 18, 2018

dnhatn Jan 18, 2018

dnhatn Jan 18, 2018

bleskes Mar 12, 2018

bleskes commented Mar 12, 2018

dnhatn left a comment

s1monw left a comment

s1monw Mar 13, 2018

bleskes Mar 13, 2018

s1monw Mar 13, 2018

bleskes Mar 13, 2018

s1monw Mar 13, 2018

bleskes Mar 13, 2018

bleskes commented Mar 14, 2018

bleskes commented Mar 15, 2018

		}


		public static void verifyHasHistoryUUID(final Directory dir) throws IOException {


		public final class EngineDiskUtils {

		private EngineDiskUtils() {

		import java.util.List;
		import java.util.Map;

Untangle Engine Constructor logic #28245

Untangle Engine Constructor logic #28245

Conversation

bleskes commented Jan 16, 2018

s1monw commented Jan 17, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bleskes commented Mar 12, 2018

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

Choose a reason for hiding this comment

bleskes commented Mar 14, 2018

bleskes commented Mar 15, 2018