Replace EngineClosedException with AlreadyClosedExcpetion #22631

bleskes · 2017-01-15T20:19:06Z

EngineClosedException is a ES level exception that is used to indicate that the engine is closed when operation starts. It doesn't really add much value and we can use AlreadyClosedException from Lucene (which may already bubble if things go wrong during operations). Having two exception can just add confusion and lead to bugs, like wrong handling of EngineClosedException when dealing with document level failures. The latter was exposed by IndexWithShadowReplicasIT.

This PR also removes the AwaitFix from the IndexWithShadowReplicasIT tests (which was what cause this to be discovered). While debugging the source of the issue I found some mismatches in document uid management in the tests. The term that was passed to the engine didn't correspond to the uid in the parsed doc - those are fixed as well.

EngineClosedException is kept around for BWC. Once this goes into 5.3, I will remove it from master.

s1monw

I left one comment

s1monw · 2017-01-16T09:39:44Z

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

@@ -544,6 +544,10 @@ final Exception checkIfDocumentFailureOrThrow(final Operation operation, final E
            // and set the error in operation.setFailure. In case of environment related errors, the failure
            // is bubbled up
            isDocumentFailure = maybeFailEngine(operation.operationType().getLowercase(), failure) == false;
+            if (failure instanceof EngineClosedException) {


I am not sure what makes a EngineClosedException not a isDocumetnFailure but AlreadyClosedException does. I think we use the boolean returned from maybeFailEngine in the wrong way here. It almost seems like that we need some state handling on Operation that tells us if the document actually failed to index or if we didn't try or if we tried, succeeded but after success we failed due to concurrent closing. This entire exception based decision making here seems error prone to me.

…eption_doc_failure # Conflicts: # core/src/test/java/org/elasticsearch/index/IndexModuleTests.java # core/src/test/java/org/elasticsearch/index/shard/IndexingOperationListenerTests.java

bleskes · 2017-01-16T14:38:10Z

@s1monw as discussed, I update the PR to stop using EngineClosedException. Can you take a look? (PR title and description are adapted as well)

s1monw

left some suggestions

s1monw · 2017-01-16T14:45:54Z

core/src/main/java/org/elasticsearch/action/bulk/TransportShardBulkAction.java

@@ -388,8 +386,6 @@ private UpdateResultHolder executeUpdateRequest(UpdateRequest updateRequest, Ind
                        Exception failure = operationResult.getFailure();
                        assert failure instanceof VersionConflictEngineException
                            || failure instanceof MapperParsingException


s1monw · 2017-01-16T14:47:54Z

.../test/java/org/elasticsearch/action/support/replication/TransportReplicationActionTests.java

        return randomFrom(
            new ShardNotFoundException(shardId),
            new IndexNotFoundException(shardId.getIndex()),
            new IndexShardClosedException(shardId),
-            new EngineClosedException(shardId),
+            new AlreadyClosedException("primary is closed"),


maybe put the shard ID into the message just for completeness?

this is a test only exception, but sure

s1monw · 2017-01-16T14:51:07Z

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

@@ -1667,7 +1666,7 @@ public void onFailedEngine(String reason, @Nullable Exception failure) {
    private Engine createNewEngine(EngineConfig config) {
        synchronized (mutex) {
            if (state == IndexShardState.CLOSED) {
-                throw new EngineClosedException(shardId);
+                throw new AlreadyClosedException("can't create engine - shard is closed");


should we include the shard ID in the message in general?

I'm partial. can do...

…eption_doc_failure

bleskes · 2017-01-16T17:39:43Z

retest this please

bleskes · 2017-01-16T20:14:54Z

Thx @s1monw

* master: (131 commits) Replace EngineClosedException with AlreadyClosedExcpetion (elastic#22631) Remove HttpServer and HttpServerAdapter in favor of a simple dispatch method (elastic#22636) move ignore parameter support from yaml test client to low level rest client (elastic#22637) Fix thread safety of Stempel's token filter factory (elastic#22610) Update profile.asciidoc Wrap rest httpclient with doPrivileged blocks (elastic#22603) Revert "Add a deprecation notice to shadow replicas (elastic#22025)" Revert "Don'y use `INDEX_SHARED_FS_ALLOW_RECOVERY_ON_ANY_NODE_SETTING` directly as it triggers (many) deprecation logging" Don'y use `INDEX_SHARED_FS_ALLOW_RECOVERY_ON_ANY_NODE_SETTING` directly as it triggers (many) deprecation logging Add a deprecation notice to shadow replicas (elastic#22025) [Docs] Fix section title in profile.asciidoc ProfileResult and CollectorResult should print machine readable timing information (elastic#22561) Add replica ops with version conflict to translog Replace custom Functional interface in ElasticsearchException with CheckedFunction Make RestChannelConsumer extend CheckedConsumer<RestChannel, Exception> replace ShardSearchRequest.FilterParser functional interface with CheckedFunction replace custom functional interface with CheckedFunction in percolate module [TEST] replace SizeFunction with Function<Integer, Integer> Expose CheckedFunction Expose logs base path ...

`EngineClosedException` is a ES level exception that is used to indicate that the engine is closed when operation starts. It doesn't really add much value and we can use `AlreadyClosedException` from Lucene (which may already bubble if things go wrong during operations). Having two exception can just add confusion and lead to bugs, like wrong handling of `EngineClosedException` when dealing with document level failures. The latter was exposed by `IndexWithShadowReplicasIT`. This PR also removes the AwaitFix from the `IndexWithShadowReplicasIT` tests (which was what cause this to be discovered). While debugging the source of the issue I found some mismatches in document uid management in the tests. The term that was passed to the engine didn't correspond to the uid in the parsed doc - those are fixed as well.

All usage has been removed in #22631, which is back ported to 5.x. This means 6.x will never get it on the wire and we can remove it

bleskes added 5 commits January 15, 2017 17:56

add test

13e44e1

don't treat EngineClosedException as a document failure

fcab427

fix tests

4e629e6

IndexModuleTests

3be3fed

fix IndexShardIT

6a2f5bb

bleskes added :Engine >bug v6.0.0-alpha1 labels Jan 15, 2017

bleskes requested a review from s1monw January 15, 2017 20:19

s1monw requested changes Jan 16, 2017

View reviewed changes

bleskes added 4 commits January 16, 2017 13:07

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

7208962

…eption_doc_failure # Conflicts: # core/src/test/java/org/elasticsearch/index/IndexModuleTests.java # core/src/test/java/org/elasticsearch/index/shard/IndexingOperationListenerTests.java

replace EngineClosedException with AlreadyClosedException

57fe058

fix line length

2d75a15

fix testIndexingOperationsListeners

ecb40b3

bleskes changed the title ~~EngineClosedException shouldn't be treated as a doc level failure~~ Replace EngineClosedException with AlreadyClosedExcpetion Jan 16, 2017

bleskes added >enhancement v5.3.0 and removed >bug labels Jan 16, 2017

s1monw approved these changes Jan 16, 2017

View reviewed changes

bleskes added 2 commits January 16, 2017 17:23

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

e85813e

…eption_doc_failure

add shard id to error message

d22f6d1

bleskes merged commit d80e3ee into elastic:master Jan 16, 2017

bleskes deleted the engine_closed_exception_doc_failure branch January 16, 2017 20:14

bleskes added a commit that referenced this pull request Jan 25, 2017

Remove EngineClosedException

ed94f75

All usage has been removed in #22631, which is back ported to 5.x. This means 6.x will never get it on the wire and we can remove it

bleskes mentioned this pull request Jan 27, 2017

Make index and delete operation execute as single bulk item (backport of #21964) #22812

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace EngineClosedException with AlreadyClosedExcpetion #22631

Replace EngineClosedException with AlreadyClosedExcpetion #22631

bleskes commented Jan 15, 2017 •

edited

Loading

s1monw left a comment

s1monw Jan 16, 2017

bleskes commented Jan 16, 2017

s1monw left a comment

s1monw Jan 16, 2017

s1monw Jan 16, 2017

bleskes Jan 16, 2017

s1monw Jan 16, 2017

bleskes Jan 16, 2017

bleskes commented Jan 16, 2017

bleskes commented Jan 16, 2017

Replace EngineClosedException with AlreadyClosedExcpetion #22631

Replace EngineClosedException with AlreadyClosedExcpetion #22631

Conversation

bleskes commented Jan 15, 2017 • edited Loading

s1monw left a comment

Choose a reason for hiding this comment

s1monw Jan 16, 2017

Choose a reason for hiding this comment

bleskes commented Jan 16, 2017

s1monw left a comment

Choose a reason for hiding this comment

s1monw Jan 16, 2017

Choose a reason for hiding this comment

s1monw Jan 16, 2017

Choose a reason for hiding this comment

bleskes Jan 16, 2017

Choose a reason for hiding this comment

s1monw Jan 16, 2017

Choose a reason for hiding this comment

bleskes Jan 16, 2017

Choose a reason for hiding this comment

bleskes commented Jan 16, 2017

bleskes commented Jan 16, 2017

bleskes commented Jan 15, 2017 •

edited

Loading