Skip to content

Conversation

@tlrx
Copy link
Member

@tlrx tlrx commented Dec 3, 2020

When IndicesService is stopping on a data node, it closes every IndexService instances that are existing by calling the removeIndex(Index, IndexRemovalReason, String) method.

The IndexRemovalReason that is passed as a parameter in this case is NO_LONGER_ASSIGNED which is also the one passed in other situation like removing an index because it got assigned to another data node. The fact that the same reason is used in multiple cases make it difficult for IndexEventListener to do the distinction between an index closed because the node is shutting down and an index closed between it moved away.

This pull request changes the IndexRemovalReason used when closing the IndicesService to be CLOSED that, I think, better reflect the reason why IndexService are closed.

@tlrx tlrx added >non-issue :Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. v8.0.0 v7.11.0 labels Dec 3, 2020
@elasticmachine elasticmachine added the Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. label Dec 3, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed (Team:Distributed)

@tlrx tlrx requested a review from DaveCTurner December 3, 2020 14:33
Copy link
Contributor

@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.

Hmm today CLOSED means POST /index/_close and friends, I don't think we should combine that with "shutting down". If we need the distinction, can we add a new value for the IndexRemovalReason enum instead?

@tlrx
Copy link
Member Author

tlrx commented Dec 3, 2020

Maybe NODE_SHUTDOWN ? Or just SHUTDOWN?

@DaveCTurner
Copy link
Contributor

SHUTDOWN sounds good to me

@tlrx tlrx requested a review from DaveCTurner December 4, 2020 10:01
@tlrx
Copy link
Member Author

tlrx commented Dec 4, 2020

Thanks David, I updated the reason.

Copy link
Contributor

@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.

LGTM.

I checked for, but did not find, places where we branch based on the index removal reason which would need updating too.

@tlrx
Copy link
Member Author

tlrx commented Dec 4, 2020

Thanks David. I checked too before opening the PR and did not found anything that would need to be adapted.

@tlrx tlrx merged commit d17e714 into elastic:master Dec 4, 2020
@tlrx tlrx deleted the indicesservice-removal-reason-when-closing branch December 4, 2020 10:26
tlrx added a commit to tlrx/elasticsearch that referenced this pull request Dec 4, 2020
…65816)

When IndicesService is stopping on a data node, it closes every 
IndexService instances that are existing by calling the 
removeIndex(Index, IndexRemovalReason, String) method.

The IndexRemovalReason that is passed as a parameter in this 
case is NO_LONGER_ASSIGNED which is also the one passed
 in other situation like removing an index because it got assigned 
to another data node. The fact that the same reason is used in 
multiple cases make it difficult for IndexEventListener to do the
 distinction between an index closed because the node is 
shutting down and an index closed between it moved away.

This commit changes the IndexRemovalReason used when 
closing the IndicesService to be SHUTDOWN.
tlrx added a commit that referenced this pull request Dec 4, 2020
…65876)

When IndicesService is stopping on a data node, it closes every 
IndexService instances that are existing by calling the 
removeIndex(Index, IndexRemovalReason, String) method.

The IndexRemovalReason that is passed as a parameter in this 
case is NO_LONGER_ASSIGNED which is also the one passed
 in other situation like removing an index because it got assigned 
to another data node. The fact that the same reason is used in 
multiple cases make it difficult for IndexEventListener to do the
 distinction between an index closed because the node is 
shutting down and an index closed between it moved away.

This commit changes the IndexRemovalReason used when 
closing the IndicesService to be SHUTDOWN.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Distributed Indexing/Distributed A catch all label for anything in the Distributed Indexing Area. Please avoid if you can. >non-issue Team:Distributed (Obsolete) Meta label for distributed team (obsolete). Replaced by Distributed Indexing/Coordination. v7.11.0 v8.0.0-alpha1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants