Do not create engine under IndexShard#mutex

[dnhatn]

Today we create a new engine under IndexShard#mutex. This is not ideal because it can block the cluster state updates which require the same mutex. The constructor of InternalEngine can take some time as it implicitly builds the global ordinals if eager_global_ordinals setting is true. To solve this problem, I explored two options:

1. Move the expensive stuff from the Engine's constructor to the start method, and call start outside IndexShard#mutex. See https://github.com/elastic/elasticsearch/compare/master...dnhatn:start-engine?expand=1

2. Create engines under a new mutex in IndexShard

I prefer the second approach for it's more contained.

Closes #43699

[elasticmachine]

Pinging @elastic/es-distributed

[henningandersen]

I also prefer the second approach, since snapshot of metadata will soon need to ask for things like max seqno and global checkpoint, which could then not be initialized yet in the case we have an engine. I am thinking about this work (that is still to be done): https://github.com/elastic/elasticsearch/pull/42518/files#diff-49e1a1b834b522f4ae6997c5defe9eb0R1243.

[s1monw]

@dnhatn why can't we create an engine under a different lock just before we acquire the mutex and do some state checks. I don't think we should make such a drastic change to the concurrency model just because a ctor can be heavy. You can protect from creating two engines with a separate lock and then once it's created do the state check and change the reference? Something like this:

diff --git a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java
index a98f501946b..4897792223b 100644
--- a/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java
+++ b/server/src/main/java/org/elasticsearch/index/shard/IndexShard.java
@@ -1593,16 +1593,26 @@ public class IndexShard extends AbstractIndexShardComponent implements IndicesCl
         assert recoveryState.getRecoverySource().expectEmptyRetentionLeases() == false || getRetentionLeases().leases().isEmpty()
             : "expected empty set of retention leases with recovery source [" + recoveryState.getRecoverySource()
             + "] but got " + getRetentionLeases();
-        synchronized (mutex) {
-            verifyNotClosed();
-            assert currentEngineReference.get() == null : "engine is running";
-            // we must create a new engine under mutex (see IndexShard#snapshotStoreMetadata).
+        synchronized (this) {
             final Engine newEngine = engineFactory.newReadWriteEngine(config);
-            onNewEngine(newEngine);
-            currentEngineReference.set(newEngine);
-            // We set active because we are now writing operations to the engine; this way,
-            // if we go idle after some time and become inactive, we still give sync'd flush a chance to run.
-            active.set(true);
+            boolean success = false;
+            try {
+                synchronized (mutex) {
+                    verifyNotClosed();
+                    assert currentEngineReference.get() == null : "engine is running";
+                    // we must create a new engine under mutex (see IndexShard#snapshotStoreMetadata).
+                    onNewEngine(newEngine);
+                    currentEngineReference.set(newEngine);
+                    // We set active because we are now writing operations to the engine; this way,
+                    // if we go idle after some time and become inactive, we still give sync'd flush a chance to run.
+                    active.set(true);
+                    success = true;
+                }
+            } finally {
+                if (success == false) {
+                    newEngine.close();
+                }
+            }
         }

[dnhatn]

I started with your suggestion, but then I dropped it. I was worried about deadlock for we need to make sure the lock ordering. I've applied your suggestion in bd56da8. Can you please take another look. Thank you.

[henningandersen]

I added a couple of comments to consider. I think this can work out, but need to ponder on it a little

[henningandersen]

I wonder if moving this outside the mutex require extra care in addRefreshListener too, since otherwise we risk someone querying in between marking state POST_RECOVERY and the refresh completing, expecting to see data because of a previous call to addRefreshListener? I am not really absolutely sure this is necessary and the timing obviously have to be horrible for anything bad to happen, but wanted your thoughts on this anyway.

[dnhatn]

Well spotted. Yes, we need to maintain this happens-before relation. We can reuse the engineMutex but I prefer a separate mutex for this. See f802515.

[henningandersen]

This slightly changes behaviour to close the current engine if closing the old engine fails. While this might not really make a difference, I think I would prefer to not do that., ie. set success=true after verifyNotClosed.

[s1monw]

I think we should close it if the old engine does't close it will just cause dangling reference to an engine and a locked shared if we don't?

[dnhatn]

I pushed 67badc6 to close the new engine if we failed to install.

[s1monw]

I think we should close it if the old engine does't close it will just cause dangling reference to an engine and a locked shared if we don't?

[dnhatn]

@henningandersen @s1monw I've addressed your comments. Can you take another look? Thank you.

[s1monw]

LGTM

[henningandersen]

LGTM.

Thanks @dnhatn

[dnhatn]

@henningandersen @s1monw Thank you for reviewing.