New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ActiveShardCount should not fail when closing the index #35936
Conversation
Pinging @elastic/es-distributed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I've added a comment suggesting a different kind of test.
server/src/main/java/org/elasticsearch/action/support/ActiveShardCount.java
Show resolved
Hide resolved
@@ -317,6 +322,26 @@ public void testOpenWaitingForActiveShardsFailed() throws Exception { | |||
ensureGreen("test"); | |||
} | |||
|
|||
public void testCloseUnassignedIndex() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to avoid writing these kind of integration tests that simulate a complicated series of steps to reproduce a corner case. Maybe instead add a unit test to ActiveShardCountTests
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe instead add a unit test to
ActiveShardCountTests
?
Ok
Let's try to avoid writing these kind of integration tests that simulate a complicated series of steps to reproduce a corner case.
Well, I've been surprised that we didn't have any integration test that closes an unassigned index so I added one. On the other hand this won't play well with replicated closed indices so it's better to not add it, I agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
server/src/test/java/org/elasticsearch/action/support/ActiveShardCountTests.java
Outdated
Show resolved
Hide resolved
Thanks @ywelsch |
The ActiveShardCount is used by cluster state observers to wait for a given number of shards to be active before returning to the caller. The current implementation does not work when an index is closed while an observer is waiting on shards to be active. In this case, a NPE is thrown and the observer is never notified that the shards won't become active. This commit fixes the ActiveShardCount.enoughShardsActive() so that it does not fail when an index is closed, similarly to what is done when an index is deleted.
The
ActiveShardCount
is used by cluster state observers to wait for a given number of shards to be active before returning to the caller. The current implementation does not work when an index is closed while an observer is waiting on shards to be active. In this case, a NPE is thrown and the observer is never notified that the shards won't become active.This pull request fixes the
ActiveShardCount.enoughShardsActive()
so that it returns "OK" when an index is closed, similarly to what is done when an index is deleted. It also adds a test that would have fail with the previous behavior.