Add system data streams to feature state snapshots #75902
Conversation
server/src/main/java/org/elasticsearch/indices/SystemDataStreamDescriptor.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
Outdated
Show resolved
Hide resolved
| assertThat(createSnapshotResponse.getSnapshotInfo().dataStreams(), not(empty())); | ||
|
|
||
| // We have to delete the data stream directly, as the feature reset API doesn't clean up system data streams yet | ||
| // See https://github.com/elastic/elasticsearch/issues/75818 |
There was a problem hiding this comment.
Added a comment to the linked issue so we remember to clean up the places we have to work around this in tests. No action required here, just noting.
...s/src/internalClusterTest/java/org/elasticsearch/datastreams/SystemDataStreamSnapshotIT.java
Outdated
Show resolved
Hide resolved
...s/src/internalClusterTest/java/org/elasticsearch/datastreams/SystemDataStreamSnapshotIT.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private void assertSnapshotSuccess(CreateSnapshotResponse createSnapshotResponse) { |
There was a problem hiding this comment.
I think you can just use org.elasticsearch.snapshots.AbstractSnapshotIntegTestCase#assertSuccessful(org.elasticsearch.action.ActionFuture<org.elasticsearch.action.admin.cluster.snapshots.create.CreateSnapshotResponse>) here with minor adjustments.
There was a problem hiding this comment.
This method seems unused now?
| || featureAssociatedIndices.size() > 0 | ||
| || featureDataStreamBackingIndices.size() > 0) { | ||
|
|
||
| featureStates.add(new SnapshotFeatureInfo(featureName, featureSystemIndices)); |
There was a problem hiding this comment.
Could we have duplicate index names/data-streams in these lists (like featureSystemIndices)? I'm mainly asking because on the snapshot side we're handling everything as List while when restoring we do create sets which only really add anything if there's duplicates so either we can go with just lists on the restore end or we should use sets here?
There was a problem hiding this comment.
We implicitly eliminate duplicate feature names at node startup by constructing a map keyed on feature names, and we check for overlapping system index/datastream names with SystemIndices#checkForOverLappingPatterns. So I don't believe we should run into issues with duplicates with any of the system resources.
I am trying to remember if we want the ordered property of lists for consistency.
What do you think would be a good way to scope this fix to the PR?
There was a problem hiding this comment.
I will take a look at turning a few of these Lists into Sets:
-
featureStates,systemDataStreamNames - Other collections used only within the for-loop
There was a problem hiding this comment.
@original-brownbear I think I've addressed all the issues you raised. Do you think we're good to merge this one?
There was a problem hiding this comment.
Thanks @williamrandolph looks good for the most part :) Just a few small points that I found on re-reading this and one new question.
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/snapshots/SnapshotsService.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private void assertSnapshotSuccess(CreateSnapshotResponse createSnapshotResponse) { |
There was a problem hiding this comment.
This method seems unused now?
| return true; | ||
| } | ||
| logger.warn( | ||
| "Restoring snapshot [" |
There was a problem hiding this comment.
Use normal style logging with placeholders instead of concatenation?
Add system data streams to the "snapshot feature state" code block, so that if we're snapshotting a feature by name we grab that feature's system data streams too. Handle these data streams on the restore side as well. * Add system data streams to feature state snapshots * Don't pass system data streams through index name resolution * Don't add no-op features to snapshots * Hook in system data streams for snapshot restoration
* Add system data streams to feature state snapshots (#75902) Add system data streams to the "snapshot feature state" code block, so that if we're snapshotting a feature by name we grab that feature's system data streams too. Handle these data streams on the restore side as well. * Add system data streams to feature state snapshots * Don't pass system data streams through index name resolution * Don't add no-op features to snapshots * Hook in system data streams for snapshot restoration
|
Backport PR: #76568 |
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
* master: (868 commits) Query API key - Rest spec and yaml tests (elastic#76238) Delay shard reassignment from nodes which are known to be restarting (elastic#75606) Reenable bwc tests for elastic#76475 (elastic#76576) Set version to 7.15 in BWC code (elastic#76577) Don't remove warning headers on all failure (elastic#76434) Disable bwc tests for elastic#76475 (elastic#76541) Re-enable bwc tests (elastic#76567) Keep track of data recovered from snapshots in RecoveryState (elastic#76499) [Transform] Align transform checkpoint range with date_histogram interval for better performance (elastic#74004) EQL: Remove "wildcard" function (elastic#76099) Fix 'accept' and 'content_type' fields for search_mvt API Add persistent licensed feature tracking (elastic#76476) Add system data streams to feature state snapshots (elastic#75902) fix the error message for instance methods that don't exist (elastic#76512) ILM: Add validation of the number_of_shards parameter in Shrink Action of ILM (elastic#74219) remove dashboard only reserved role (elastic#76507) Fix Stack Overflow in UnassignedInfo in Corner Case (elastic#76480) Add (Extended)KeyUsage KeyUsage, CipherSuite & Protocol to SSL diagnostics (elastic#65634) Add recovery from snapshot to tests (elastic#76535) Reenable BwC Tests after elastic#76532 (elastic#76534) ...
This PR adds system data streams to the "snapshot feature state" code block, so that if we're snapshotting a feature by name we grab that feature's system data streams too.
Fixes #75860