Support failure store in snapshot and restore operations#107281
Support failure store in snapshot and restore operations#107281elasticsearchmachine merged 22 commits intoelastic:mainfrom
Conversation
gmarouli
added
>non-issue
:StorageEngine/Data streams
gmarouli
changed the title
|
We believe the test failure is not related to the failure store changes but it's a result of the restoration of the |
elasticsearchmachine
added
the
Team:Data Management (obsolete)
|
Pinging @elastic/es-data-management (Team:Data Management) |
nielsbauman
left a comment
nielsbauman
left a comment
There was a problem hiding this comment.
Awesome that you got this to work with relatively few changes! I only left some comments on the functionality itself for now. I'll have another look at the tests some other time.
server/src/main/java/org/elasticsearch/action/support/IndicesOptions.java
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/cluster/metadata/DataStreamFailureStoreDefinition.java
Show resolved
Hide resolved
jbaiera
left a comment
jbaiera
left a comment
There was a problem hiding this comment.
Some initial comments and questions. Looking good though, frustrating thing that cluster state problem.
| Stream.of(requestIndices, featureStateDataStreams).flatMap(Collection::stream).toArray(String[]::new), | ||
| IndicesOptions.fromOptions(true, true, true, true) | ||
| DataStream.isFailureStoreFeatureFlagEnabled() | ||
| ? IndicesOptions.lenientExpandIncludeFailureStore() |
There was a problem hiding this comment.
Looking at the filterIndices function - it doesn't seem to actually make any use of whether to include failure stores or not. Could we leave this unchanged?
There was a problem hiding this comment.
Good point, I made it more explicit in the javadoc and only kept the IndicesOptions.lenientExpand().
| return renameIndex(index, request, DataStream.BACKING_INDEX_PREFIX); | ||
| } | ||
|
|
||
| private static String renameFailureIndex(String index, RestoreSnapshotRequest request) { |
There was a problem hiding this comment.
This looks identical to the above method. I personally think it scans a little better while reading, but the duplication makes me think that it's probably better to merge all of it and control the logic based on which prefix is present (or not present for non-data streams)
I think we're also losing a check here in renameIndex() if we're restoring an index that should not be renamed. Not sure from just reviewing if that check is important right now, but it existed beforehand and makes me worry about its removal.
There was a problem hiding this comment.
I thought I commented this already but I guess I didn't; the if-statements in renameBackingIndex and renameFailureIndex are already covered by the renameIndex above. So, after removing those if-statements, it probably makes even more sense to merge the methods.
There was a problem hiding this comment.
Merged the methods. Let me know if it matches your expectations
| assertEquals(dataStreamName, updateDataStream.getName()); | ||
| assertEquals(Collections.singletonList(updatedIndex), updateDataStream.getIndices()); | ||
| assertEquals(List.of(updatedBackingIndex), updateDataStream.getIndices()); | ||
| assertEquals(List.of(updatedFailureIndex), updateDataStream.getFailureIndices()); |
There was a problem hiding this comment.
Will these tests work if the feature flag is disabled? I recall that we enable the flag just about everywhere but I can't remember if there are still cases where the tests run without it on (like for releases).
There was a problem hiding this comment.
I thought the unit tests always have the feature flags enabled, isn't this the case?
...reams/src/internalClusterTest/java/org/elasticsearch/datastreams/DataStreamsSnapshotsIT.java
Outdated
Show resolved
Hide resolved
|
@elasticmachine update branch |
|
@elasticmachine update branch |
|
@elasticmachine update branch |
gmarouli
added
the
auto-merge-without-approval
5 participants
In this PR we update the snapshot and restore implementations to include the failure store of a data stream when a data stream is being snapshot.