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
Data streams fix failure store delete #104281
Data streams fix failure store delete #104281
Conversation
Pinging @elastic/es-data-management (Team:Data Management) |
Hi @jbaiera, I've created a changelog YAML for you. |
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, I left one question but I think either way could be argued, so I'll leave it up to you.
@@ -155,6 +155,9 @@ static ClusterState removeDataStream( | |||
DataStream dataStream = currentState.metadata().dataStreams().get(dataStreamName); | |||
assert dataStream != null; | |||
backingIndicesToRemove.addAll(dataStream.getIndices()); | |||
if (DataStream.isFailureStoreEnabled()) { |
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.
This is actually one of those rare cases I think we don't want to check the flag. For example, if someone were to set the flag to true (via system properties), then play with it, then set it back to false, deleting the data stream would leave the failure store indices hanging. Do you think we need the flag here?
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.
I added it for consistency's sake. If you try this feature right now and then disable it afterward I'm pretty sure cluster state breaks to begin with since we conditionally execute the parsing code to pull this data based on the flag. I'd be ok with removing this flag check here because normal execution should see all this lists as empty. It makes me wonder if we should revisit how the cluster state parsing is handled.
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.
I went ahead and removed the flag, save us some keystrokes later.
💚 All backports created successfully
Questions ?Please refer to the Backport tool documentation |
This PR adds the any failure store indices to the list of indices to be deleted when deleting a data stream. (cherry picked from commit 2a79d78)
This PR adds the any failure store indices to the list of indices to be deleted when deleting a data stream.
Data streams that have failure stores configured do not currently delete them when the data stream is deleted. This PR adds the any failure store indices to the list of indices to be deleted when deleting a data stream.