-
Notifications
You must be signed in to change notification settings - Fork 25.6k
Converting an Existing Data Stream to a System DataStream is Broken #121392
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
Changes from 5 commits
6289eb9
5e92c45
a1b6ceb
251e88b
9a4ee2c
4ef97b4
61f9c4f
1a7a5d5
19f9700
78e8059
d20b021
ead1cc8
4993fdc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| pr: 121392 | ||
| summary: Converting an Existing Data Stream to a System `DataStream` is Broken | ||
| area: Infra/Core | ||
| type: bug | ||
| issues: [] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At https://github.com/elastic/elasticsearch/pull/121392/files#diff-1123b904a4743231e7321993f73a75ae603c6658b1e6acf6bf8656b69370a410L70 we currently only check for indices that should become system indices. Should we also check for data streams there? It seems that if a data stream exists, it always has at least one backing index. If the data stream is non-system, the backing index will also be non-system. Then, when the data stream is marked as system, the index will be picked up by the task, and both will be converted to system ones. Does this approach make sense, or should we explicitly check for data streams as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, good question. For now that is the case, but we have talked about changing this one day but there are no concrete plans. If I am not mistaken, this is just looping over one more data set, so I think it would be better to be "thorough" and avoid surprises in the future. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'll add check for data streams here as well. Thank you! |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -129,14 +129,28 @@ private void submitUnbatchedTask(@SuppressWarnings("SameParameterValue") String | |
| } | ||
|
|
||
| // visible for testing | ||
| SystemIndexMetadataUpdateTask getTask() { | ||
| return new SystemIndexMetadataUpdateTask(); | ||
| ClusterState executeMetadataUpdateTask(ClusterState clusterState) { | ||
| return new SystemIndexMetadataUpdateTask().execute(clusterState); | ||
| } | ||
|
|
||
| public class SystemIndexMetadataUpdateTask extends ClusterStateUpdateTask { | ||
| private class SystemIndexMetadataUpdateTask extends ClusterStateUpdateTask { | ||
|
|
||
| @Override | ||
| public ClusterState execute(ClusterState currentState) throws Exception { | ||
| public ClusterState execute(ClusterState currentState) { | ||
| List<IndexMetadata> updatedMetadata = updateIndices(currentState); | ||
| List<DataStream> updatedDataStreams = updateDataStreams(currentState); | ||
|
|
||
| if (updatedMetadata.isEmpty() == false || updatedDataStreams.isEmpty() == false) { | ||
| Metadata.Builder builder = Metadata.builder(currentState.metadata()); | ||
| updatedMetadata.forEach(idxMeta -> builder.put(idxMeta, true)); | ||
| updatedDataStreams.forEach(builder::put); | ||
|
|
||
| return ClusterState.builder(currentState).metadata(builder).build(); | ||
| } | ||
| return currentState; | ||
| } | ||
|
|
||
| private List<IndexMetadata> updateIndices(ClusterState currentState) { | ||
| final Map<String, IndexMetadata> indexMetadataMap = currentState.metadata().indices(); | ||
| final List<IndexMetadata> updatedMetadata = new ArrayList<>(); | ||
| for (Map.Entry<String, IndexMetadata> entry : indexMetadataMap.entrySet()) { | ||
|
|
@@ -172,13 +186,23 @@ public ClusterState execute(ClusterState currentState) throws Exception { | |
| updatedMetadata.add(builder.build()); | ||
| } | ||
| } | ||
| return updatedMetadata; | ||
| } | ||
|
|
||
| if (updatedMetadata.isEmpty() == false) { | ||
| final Metadata.Builder builder = Metadata.builder(currentState.metadata()); | ||
| updatedMetadata.forEach(idxMeta -> builder.put(idxMeta, true)); | ||
| return ClusterState.builder(currentState).metadata(builder).build(); | ||
| private List<DataStream> updateDataStreams(ClusterState currentState) { | ||
| List<DataStream> updatedDataStreams = new ArrayList<>(); | ||
| for (DataStream dataStream : currentState.getMetadata().dataStreams().values()) { | ||
| boolean shouldBeSystem = systemIndices.isSystemDataStream(dataStream.getName()); | ||
| if (dataStream.isSystem() != shouldBeSystem) { | ||
| DataStream.Builder dataStreamBuilder = dataStream.copy().setSystem(shouldBeSystem); | ||
| if (shouldBeSystem) { | ||
| dataStreamBuilder.setHidden(true); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the Do we likewise need to update the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, good question, we update generations when the backing or failure indices get modified (added, deleted, etc) but I do not think the update of system property qualifies as such. @dakrone do you agree? |
||
| } | ||
|
|
||
| updatedDataStreams.add(dataStreamBuilder.build()); | ||
| } | ||
| } | ||
| return currentState; | ||
| return updatedDataStreams; | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.