Skip to content

Relax Overly Strict Assertion in TransportShardBulkAction#40940

Merged
original-brownbear merged 3 commits intoelastic:masterfrom
original-brownbear:40933
Apr 8, 2019
Merged

Relax Overly Strict Assertion in TransportShardBulkAction#40940
original-brownbear merged 3 commits intoelastic:masterfrom
original-brownbear:40933

Conversation

@original-brownbear
Copy link
Copy Markdown
Contributor

@original-brownbear original-brownbear commented Apr 8, 2019 •
edited
Loading

This exception trips the assertion:

org.elasticsearch.index.shard.IndexShardClosedException: CurrentState[CLOSED] operation only allowed when not closed
  1>    at org.elasticsearch.index.shard.IndexShard.verifyNotClosed(IndexShard.java:1598) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.index.shard.IndexShard.verifyNotClosed(IndexShard.java:1592) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.index.shard.IndexShard.sync(IndexShard.java:2796) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportWriteAction$AsyncAfterWriteAction.run(TransportWriteAction.java:356) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportWriteAction$WritePrimaryResult.<init>(TransportWriteAction.java:152) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.bulk.TransportShardBulkAction$2.finishRequest(TransportShardBulkAction.java:195) ~[elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.bulk.TransportShardBulkAction$2.doRun(TransportShardBulkAction.java:168) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.bulk.TransportShardBulkAction.performOnPrimary(TransportShardBulkAction.java:198) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.bulk.TransportShardBulkAction.shardOperationOnPrimary(TransportShardBulkAction.java:116) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.bulk.TransportShardBulkAction.shardOperationOnPrimary(TransportShardBulkAction.java:77) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportReplicationAction$PrimaryShardReference.perform(TransportReplicationAction.java:927) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.ReplicationOperation.execute(ReplicationOperation.java:104) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportReplicationAction$AsyncPrimaryAction.runWithPrimaryShardReference(TransportReplicationAction.java:364) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportReplicationAction$AsyncPrimaryAction.lambda$doRun$0(TransportReplicationAction.java:311) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.ActionListener$1.onResponse(ActionListener.java:62) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.index.shard.IndexShardOperationPermits.acquire(IndexShardOperationPermits.java:269) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.index.shard.IndexShardOperationPermits.acquire(IndexShardOperationPermits.java:236) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.index.shard.IndexShard.acquirePrimaryOperationPermit(IndexShard.java:2513) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportReplicationAction.acquirePrimaryOperationPermit(TransportReplicationAction.java:865) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportReplicationAction$AsyncPrimaryAction.doRun(TransportReplicationAction.java:310) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.action.support.replication.TransportReplicationAction.handlePrimaryRequest(TransportReplicationAction.java:273) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.transport.RequestHandlerRegistry.processMessageReceived(RequestHandlerRegistry.java:63) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.transport.TransportService$7.doRun(TransportService.java:703) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:747) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:37) [elasticsearch-8.0.0-SNAPSHOT.jar:8.0.0-SNAPSHOT]
  1>    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
  1>    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
  1>    at java.lang.Thread.run(Thread.java:835) [?:?]

@original-brownbear
Remove Overly Strict Assertion in TransportShardBulkAction
7c3e3cc 
* In #39793 this assertion was added under the assumption that no exceptions would be thrown in this method, which turned out not to be correct and at the very least `org.elasticsearch.index.shard.IndexShardClosedException` can be thrown by `org.elasticsearch.index.shard.IndexShard.sync`
* Closes #40933
@original-brownbear original-brownbear added >non-issue :Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. v8.0.0 v7.2.0 labels Apr 8, 2019
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Apr 8, 2019

Pinging @elastic/es-distributed

@original-brownbear
Copy link
Copy Markdown
Contributor Author

original-brownbear commented Apr 8, 2019

Another example of why this assertion wasn't a good idea https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request-1/11667/ :)

@original-brownbear original-brownbear changed the title Remove Overly Strict Assertion in TransportShardBulkAction Relax Overly Strict Assertion in TransportShardBulkAction Apr 8, 2019
@original-brownbear
Copy link
Copy Markdown
Contributor Author

original-brownbear commented Apr 8, 2019

@dnhatn this fix seems to empirically fix this just fine for me, but I'm a little worried about the exception that's causing this in the first place:

org.elasticsearch.index.shard.IndexShardClosedException

could it be some mistake snuck into #39793 that allows for the shard to be closed in the middle of the bulk operation here?

@original-brownbear
Copy link
Copy Markdown
Contributor Author

original-brownbear commented Apr 8, 2019

Jenkins run elasticsearch-ci/2

DaveCTurner
DaveCTurner approved these changes Apr 8, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@DaveCTurner DaveCTurner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, LGTM, this will help with ongoing test failures. I would still like @dnhatn to look at this too as the failure handling in this area is subtle.

@original-brownbear
Copy link
Copy Markdown
Contributor Author

original-brownbear commented Apr 8, 2019

@DaveCTurner thanks merging now to fix tests then, but yea @dnhatn please take a look when you can :)

@original-brownbear original-brownbear merged commit 6e4defd into elastic:master Apr 8, 2019
@original-brownbear original-brownbear deleted the 40933 branch April 8, 2019 12:48
dnhatn
dnhatn reviewed Apr 8, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@dnhatn dnhatn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks @original-brownbear.

jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Apr 8, 2019
@jasontedor
Merge remote-tracking branch 'elastic/master' into opt-out-query-cach…
df86b70 
…e-unsafe-publication

* elastic/master:
  Update contributing docs to reflect JDK 11 (elastic#40955)
  Docs: Simplifying setup by using module configuration variant syntax (elastic#40879)
  Unmute CreateIndexIT testCreateAndDeleteIndexConcurrently CI failures (elastic#40960)
  Revert "Short-circuit rebalancing when disabled (elastic#40942)"
  Mute EnableAllocationShortCircuitTests
  SQL: Refactor args verification of In & conditionals (elastic#40916)
  Avoid sharing source directories as it breaks intellij (elastic#40877)
  Short-circuit rebalancing when disabled (elastic#40942)
  SQL: Prefer resultSets over exceptions in metadata (elastic#40641)
  Mute ClusterPrivilegeTests.testThatSnapshotAndRestore
  Fix Race in AsyncTwoPhaseIndexerTests.testStateMachine (elastic#40947)
  Relax Overly Strict Assertion in TransportShardBulkAction (elastic#40940)
@original-brownbear original-brownbear mentioned this pull request Apr 11, 2019
Merged
gurkankaymak pushed a commit to gurkankaymak/elasticsearch that referenced this pull request May 27, 2019
@original-brownbear
Relax Overly Strict Assertion in TransportShardBulkAction (elastic#40940
4f6b4bb 
)

* Remove Overly Strict Assertion in TransportShardBulkAction

* In elastic#39793 this assertion was added under the assumption that no exceptions would be thrown in this method, which turned out not to be correct and at the very least `org.elasticsearch.index.shard.IndexShardClosedException` can be thrown by `org.elasticsearch.index.shard.IndexShard.sync`
* Closes elastic#40933
@jakelandis jakelandis removed the v8.0.0 label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dnhatn dnhatn dnhatn left review comments

@DaveCTurner DaveCTurner DaveCTurner approved these changes

Assignees

No one assigned

Labels

:Distributed/Distributed A catch all label for anything in the Distributed Area. Please avoid if you can. >non-issue v7.2.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AnnotationIndexIT.testCreatedWhenAfterOtherMlIndex fails

5 participants

@original-brownbear @elasticmachine @DaveCTurner @dnhatn @jakelandis