[Backport 2.19] Add data stream backing-index modify API and attach_to_data_stream restore option (manual)

[sokdak]

Description

Manual backport of two main-line commits to 2.19:

• Add API to modify a data stream's backing indices #22487 — POST /_data_stream/_modify with add_backing_index / remove_backing_index (metadata-only, applied atomically in a single cluster-state update)
• Add option to attach restored backing indices to their data stream #22539 — attach_to_data_stream option on the snapshot restore API, which attaches a restored .ds-<stream>-NNNNNN index to a pre-existing data stream of the same name in the same cluster-state update

Backported manually rather than by the bot because the two commits do not compile on this
line unchanged. Both cherry-picks apply to 2.19 with zero conflicts; the compile
failures only surface afterwards, and are addressed in a separate commit so the backport
content stays reviewable against the originals.

Why this matters on 2.19

Deleting a data stream backing index silently detaches it, and until #22487/#22539 there was no way to put it back — _data_stream/_modify returns 405 and attach_to_data_stream is an unknown parameter. Any workflow that takes a backing index out of the cluster and restores it (searchable-snapshot mounts, cold-to-warm moves, restoring a RED backing index from a snapshot) leaves the data behind as an index that is no longer reachable by the data stream name. #8271 describes the same problem from the cold-to-warm restore side.

2.19 API adaptations (commit 3)

main	2.19
ClusterManagerTask enum + registerClusterManagerTask(ClusterManagerTask, boolean)	no such class; ClusterManagerTaskKeys String constants + registerClusterManagerTask(String, boolean). Adds MODIFY_DATA_STREAM_KEY = "modify-data-stream", same wire string and throttling behaviour, matching MetadataCreateDataStreamService
DataStreamAction as a Java record	server compiles with -source 11; converted to a final class with an explicit canonical constructor (keeping the requireNonNull checks), identical accessor names, hand-written equals/hashCode/toString
org.opensearch.action.support.clustermanager.Acknowledged{Request,Response}	org.opensearch.action.support.master.* (matches CreateDataStreamAction)
org.opensearch.transport.client.node.NodeClient	org.opensearch.client.node.NodeClient (matches RestCreateDataStreamAction)
clusterManagerNodeTimeout(TimeValue)	masterNodeTimeout(TimeValue)

No production code and no tests were dropped. Features adjacent to the backported hunks that do not exist on this line (AliasWriteIndexPolicy in RestoreSnapshotRequest, ViewService in Node, wlm_stats_list in RestHighLevelClientTests) were excluded rather than pulled in.

The version constant — please read (commit 4)

#22539 guards the RestoreSnapshotRequest.attachToDataStream wire field on Version.V_3_8_0, which does not exist here. The guard has to key on a version that no already-released node reports. Keying it on Version.CURRENT (== V_2_19_6) is not a degraded workaround — it is an outage:

Testing

Run on this branch, JDK 21, macOS arm64:

• :server:test — 15423 tests, 0 failures, 0 errors, 67 skipped across 1841 suites
  (BUILD SUCCESSFUL; counts read from the JUnit XML, not the console)
• targeted units — 74/74, including VersionTests 27/27 (the suite that polices the bump),
  RestoreSnapshotRequestTests, RestoreServiceTests, MetadataDataStreamsServiceTests,
  DataStreamTests, ModifyDataStreamsRequestTests
• :server:internalClusterTest — ModifyDataStreamsIT 7/7 and
  DataStreamRestoreAutoAttachIT 2/2, i.e. the suites the two PRs bring with them

Worth flagging for reviewers: RestoreSnapshotRequestTests cannot catch a wrong version
guard. It extends AbstractWireSerializingTestCase, and AbstractWireTestCase hardcodes the round-trip at Version.CURRENT, so both guards are trivially symmetric there. Reverting the guard to the known-bad Version.CURRENT form still yields tests=3 failures=0. Mixed-version safety on this change can only be established at the node level.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit fa3f261)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 Multiple PR themes Sub-PR theme: Backport #22487: POST /_data_stream/_modify API Relevant files: server/src/main/java/org/opensearch/action/admin/indices/datastream/DataStreamAction.java server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java server/src/main/java/org/opensearch/cluster/metadata/MetadataDataStreamsService.java server/src/main/java/org/opensearch/rest/action/admin/indices/RestModifyDataStreamsAction.java server/src/main/java/org/opensearch/cluster/service/ClusterManagerTaskKeys.java server/src/main/java/org/opensearch/cluster/metadata/DataStream.java rest-api-spec/src/main/resources/rest-api-spec/api/indices.modify_data_stream.json server/src/internalClusterTest/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsIT.java server/src/test/java/org/opensearch/action/admin/indices/datastream/DataStreamActionTests.java server/src/test/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsRequestTests.java server/src/test/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsTransportActionTests.java server/src/test/java/org/opensearch/cluster/metadata/MetadataDataStreamsServiceTests.java server/src/test/java/org/opensearch/cluster/metadata/DataStreamTests.java server/src/test/java/org/opensearch/rest/action/admin/indices/RestModifyDataStreamsActionTests.java Sub-PR theme: Backport #22539: attach_to_data_stream restore option Relevant files: server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilder.java server/src/main/java/org/opensearch/snapshots/RestoreService.java server/src/test/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestBuilderTests.java server/src/test/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequestTests.java server/src/test/java/org/opensearch/snapshots/RestoreServiceTests.java server/src/internalClusterTest/java/org/opensearch/snapshots/DataStreamRestoreAutoAttachIT.java

⚡ Recommended focus areas for review Numeric parse of counter can throw NumberFormatException backingIndexCounterOrMin calls Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)) after only checking that parseDataStreamName(indexName) matches the data stream name. parseDataStreamName requires the suffix after the last '-' to be all digits, but that suffix can be arbitrarily long. Any input whose numeric counter exceeds Long.MAX_VALUE (e.g. .ds-logs-foo-99999999999999999999) will cause Long.parseLong to throw NumberFormatException, propagating out of addBackingIndex and the modify/restore code paths. Consider limiting counter length in parseDataStreamName (or catching the parse failure and treating it as non-convention) to keep the API robust against maliciously or accidentally named indices. static long backingIndexCounterOrMin(String dataStreamName, String indexName) { if (dataStreamName.equals(parseDataStreamName(indexName)) == false) { return Long.MIN_VALUE; } return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); } toXContent emits a field older parsers do not know toXContent now unconditionally writes attach_to_data_stream, but source(Map) only accepts this key on the branch added by this PR; older-version clients that consume the produced XContent (e.g., via reindex-from-remote-style flows or persisted snapshot-restore request bodies) and try to parse it back with source(Map) will hit the "Unknown parameter " + name branch and fail. Consider emitting the field only when non-default (if (attachToDataStream) {...}) to preserve backward-compatible XContent shape for the default case. builder.field("attach_to_data_stream", attachToDataStream); equals() dangling `&&` may be a merge artifact The equals block now ends with && Objects.equals(sourceRemoteTranslogRepository, that.sourceRemoteTranslogRepository) && attachToDataStream == that.attachToDataStream; followed by return equals;. Verify that the assignment target equals still exists and that this compiles as a single expression — as shown in the diff the last line stands alone with a leading &&, which suggests either a diff-rendering artifact or a broken chain. If broken at compile time, restore functionality is silently degraded (equals would ignore the new field or a pre-existing field). && Objects.equals(sourceRemoteTranslogRepository, that.sourceRemoteTranslogRepository) && attachToDataStream == that.attachToDataStream; return equals; Unknown top-level fields silently accepted In parseActions, the else that throws on unexpected fields is only reached when the current token is a non-FIELD_NAME, non-matching-array token. A top-level FIELD_NAME whose name is not actions sets currentFieldName and the loop advances to its value token; if that value is a scalar/object (not an array), the branch conditions all fail and no error is raised. As written, requests like {"unknown":"x","actions":[...]} may silently ignore unknown. Consider explicitly rejecting any field name other than actions. XContentParser.Token token = parser.nextToken(); if (token != XContentParser.Token.START_OBJECT) { throw new IllegalArgumentException("expected an object with an [actions] array"); } String currentFieldName = null; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); } else if ("actions".equals(currentFieldName) && token == XContentParser.Token.START_ARRAY) { while (parser.nextToken() != XContentParser.Token.END_ARRAY) { actions.add(DataStreamAction.fromXContent(parser)); } } else { throw new IllegalArgumentException("unexpected field [" + currentFieldName + "], only [actions] is supported"); } }

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to fa3f261

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Hide restored backing indices on attach Unlike MetadataDataStreamsService.applyAddBackingIndex, this path does not mark the newly attached backing index hidden. Every backing index of a data stream is expected to be hidden (as also asserted by ModifyDataStreamsIT.testDetachUnhidesAndReattachHidesBackingIndex), so a restored index that was not previously hidden will remain visible after being attached, inconsistent with the modify API and with normal rollover behavior. Consider updating the settings to set INDEX_HIDDEN to true on attach. server/src/main/java/org/opensearch/snapshots/RestoreService.java [995-1011] DataStream currentDs = updatedDataStreams.get(streamName); IndexMetadata restoredIndexMetadata = metadata.get(restoredIndexName); if (currentDs == null || restoredIndexMetadata == null) { continue; } if (currentDs.getIndices().contains(restoredIndexMetadata.getIndex())) { continue; } -// A backing index must map the timestamp field as a date, or data stream search breaks. MetadataDataStreamsService.validateTimestampFieldMapping(restoredIndexMetadata, currentDs.getTimeStampField().getName()); +if (IndexMetadata.INDEX_HIDDEN_SETTING.get(restoredIndexMetadata.getSettings()) == false) { + IndexMetadata hidden = IndexMetadata.builder(restoredIndexMetadata) + .settings(Settings.builder().put(restoredIndexMetadata.getSettings()).put(IndexMetadata.SETTING_INDEX_HIDDEN, true)) + .settingsVersion(restoredIndexMetadata.getSettingsVersion() + 1) + .build(); + metadata.put(hidden, true); + restoredIndexMetadata = hidden; +} updatedDataStreams.put(streamName, currentDs.addBackingIndex(restoredIndexMetadata.getIndex())); Suggestion importance[1-10]: 6 __ Why: Valid observation: restored indices attached to data streams should be hidden to match the invariant enforced by MetadataDataStreamsService.applyAddBackingIndex. However, restored data stream backing indices likely already have the hidden setting from their original snapshot metadata, reducing practical impact.	Low
	Improve error for non-array actions field When the top-level field name is actions but its value is not an array (e.g. a string or object), the error thrown is misleading: it still says "unexpected field [actions], only [actions] is supported". Distinguish the two cases so the user gets a message that describes the actual problem (wrong value type for actions). server/src/main/java/org/opensearch/rest/action/admin/indices/RestModifyDataStreamsAction.java [60-69] String currentFieldName = null; while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); - } else if ("actions".equals(currentFieldName) && token == XContentParser.Token.START_ARRAY) { + } else if ("actions".equals(currentFieldName)) { + if (token != XContentParser.Token.START_ARRAY) { + throw new IllegalArgumentException("[actions] must be an array"); + } while (parser.nextToken() != XContentParser.Token.END_ARRAY) { actions.add(DataStreamAction.fromXContent(parser)); } } else { throw new IllegalArgumentException("unexpected field [" + currentFieldName + "], only [actions] is supported"); } } Suggestion importance[1-10]: 4 __ Why: A reasonable UX improvement: distinguishing "actions is wrong type" from "unknown field" gives clearer error messages. However, the test testActionsFieldThatIsNotAnArrayIsRejected currently asserts the existing (misleading) message, so this would require test updates. Minor readability/UX improvement.	Low
	Guard against numeric overflow in counter parsing Long.parseLong can throw NumberFormatException for very large counters (e.g. > Long.MAX_VALUE) even though parseDataStreamName already validated the suffix is all digits. Since the test testAddBackingIndexWithCounterAboveIntMax uses Integer.MAX_VALUE + 5 this is fine, but a 20+ digit numeric counter would crash sorting. Guard against overflow by catching the exception and returning Long.MAX_VALUE so it still sorts last without breaking the API. server/src/main/java/org/opensearch/cluster/metadata/DataStream.java [175-180] static long backingIndexCounterOrMin(String dataStreamName, String indexName) { if (dataStreamName.equals(parseDataStreamName(indexName)) == false) { return Long.MIN_VALUE; } - return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + try { + return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + } catch (NumberFormatException e) { + return Long.MAX_VALUE; + } } Suggestion importance[1-10]: 3 __ Why: The suggestion addresses an edge case (20+ digit counters) that is unlikely in practice since backing index counters are constrained by rollover mechanics. The parseDataStreamName already validates digits, and the overflow path would require extremely long counters. Low practical impact.	Low

---

Previous suggestions

Suggestions up to commit 698afcd

Category	Suggestion	Impact
General	Hide attached backing indices on restore attachRestoredBackingIndices calls addBackingIndex which does not mark the index hidden, unlike MetadataDataStreamsService.applyAddBackingIndex. Restored indices attached to a data stream will therefore remain visible, inconsistent with all other backing indices and with the modify API's behavior. Consider updating the index metadata to set index.hidden=true here, or reusing the service's logic. server/src/main/java/org/opensearch/snapshots/RestoreService.java [1005-1010] if (currentDs.getIndices().contains(restoredIndexMetadata.getIndex())) { continue; } // A backing index must map the timestamp field as a date, or data stream search breaks. MetadataDataStreamsService.validateTimestampFieldMapping(restoredIndexMetadata, currentDs.getTimeStampField().getName()); +if (IndexMetadata.INDEX_HIDDEN_SETTING.get(restoredIndexMetadata.getSettings()) == false) { + IndexMetadata.Builder hidden = IndexMetadata.builder(restoredIndexMetadata) + .settings(Settings.builder().put(restoredIndexMetadata.getSettings()).put(IndexMetadata.SETTING_INDEX_HIDDEN, true)) + .settingsVersion(restoredIndexMetadata.getSettingsVersion() + 1); + metadata.put(hidden); + restoredIndexMetadata = hidden.build(); +} updatedDataStreams.put(streamName, currentDs.addBackingIndex(restoredIndexMetadata.getIndex())); Suggestion importance[1-10]: 7 __ Why: A valid consistency concern: the modify API hides attached backing indices, but the restore-attach path does not, potentially leaving restored indices visible unlike other backing indices. This could be a real behavioral inconsistency worth addressing.	Medium
	Guard counter parsing against overflow parseDataStreamName only accepts pure numeric suffixes, but Long.parseLong on the raw suffix after lastIndexOf('-') may still throw NumberFormatException for pathological inputs where the substring passes the digit check but overflows long (e.g. more than 19 digits). Consider catching NumberFormatException and returning Long.MIN_VALUE to avoid propagating a runtime error during sorting when unusual index names are present. server/src/main/java/org/opensearch/cluster/metadata/DataStream.java [175-180] static long backingIndexCounterOrMin(String dataStreamName, String indexName) { if (dataStreamName.equals(parseDataStreamName(indexName)) == false) { return Long.MIN_VALUE; } - return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + try { + return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + } catch (NumberFormatException e) { + return Long.MIN_VALUE; + } } Suggestion importance[1-10]: 3 __ Why: The suggestion is defensive but not critical: parseDataStreamName already restricts the suffix to digits, so overflow would only occur for names with more than 18 digits, which is unlikely in practice. Minor robustness improvement.	Low
	Clarify shared-index validation invariant validateNoSharedBackingIndices builds finalStreams from currentStreams overlaid with updatedStreams, but currentStreams is the pre-update snapshot. If an action removes a stream membership, currentStreams still shows the old (unmodified) stream data — however since updatedStreams overrides by name, this is fine only for mutated streams. Streams not touched by this request are correctly represented by currentStreams. This is correct, but consider clarifying that the invariant relies on updatedStreams fully replacing each mutated stream's index list. server/src/main/java/org/opensearch/cluster/metadata/MetadataDataStreamsService.java [155-156] Map<String, DataStream> finalStreams = new HashMap<>(currentStreams); -finalStreams.putAll(updatedStreams); +finalStreams.putAll(updatedStreams); // updatedStreams fully replaces mutated streams; untouched streams come from currentStreams Suggestion importance[1-10]: 2 __ Why: This is only a documentation/comment clarification and does not change the behavior; low impact.	Low
Possible issue	Verify version guard consistency across branches The version guard uses V_2_19_7, but this field is also being introduced on main/higher branches in a separate change. Confirm that all target branches use the same first-unreleased-version constant for this field, otherwise a 2.19.7 node writing attachToDataStream will mismatch a higher-version peer that expects the field at a different guard version, causing wire-format corruption between mixed-version peers. server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java [183-185] +if (in.getVersion().onOrAfter(Version.V_2_19_7)) { + attachToDataStream = in.readBoolean(); +} - Suggestion importance[1-10]: 5 __ Why: Cross-branch version consistency for wire-format guards is a legitimate concern for BWC, but the suggestion only asks to verify without a concrete code change.	Low

Suggestions up to commit 698afcd

Category	Suggestion	Impact
Possible issue	Verify wire-version guard across branches The version guard uses V_2_19_7, but this is a 2.19 backport of a change that also lives on main/2.x. If the non-backport (main) branch guards the same field with a different version constant (e.g., V_3_x_0), nodes on 2.19.7 and the main branch will disagree on the byte layout and fail to communicate. Verify the constant matches the guard used on all branches where this field is introduced; otherwise mixed-version clusters will break (new writer -> old reader will read a spurious boolean, old writer -> new reader will miss the field). server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java [183-185] +if (in.getVersion().onOrAfter(Version.V_2_19_7)) { + attachToDataStream = in.readBoolean(); +} - Suggestion importance[1-10]: 6 __ Why: Valid concern about cross-branch version guard consistency for BWC, but it's a verification-only suggestion without concrete code changes (the existing_code and improved_code are identical).	Low
General	Reject empty parsed data stream name The check counterDash <= BACKING_INDEX_PREFIX.length() rejects the case where the last dash sits exactly at the prefix boundary, but does not guarantee the stream-name segment is non-empty when the prefix already ends with '-'. If BACKING_INDEX_PREFIX ends with '-' (typical: .ds-), a name like .ds--000001 would parse to an empty stream name. Guard against an empty extracted stream name to avoid returning a bogus empty string. server/src/main/java/org/opensearch/cluster/metadata/DataStream.java [190-193] int counterDash = indexName.lastIndexOf('-'); if (counterDash <= BACKING_INDEX_PREFIX.length()) { return null; } +String streamName = indexName.substring(BACKING_INDEX_PREFIX.length(), counterDash); +if (streamName.isEmpty()) { + return null; +} Suggestion importance[1-10]: 5 __ Why: Minor edge-case hardening for pathological index names like .ds--000001. Unlikely to occur in practice but adds robustness to the parser.	Low
	Handle overflow when parsing counter Long.parseLong will throw NumberFormatException if the numeric suffix exceeds Long.MAX_VALUE digits, which is a plausible input from user-supplied index names. Since parseDataStreamName only validates that the suffix consists of digits (any length), a very long digit string will crash here. Wrap the parse or bound the digit count in parseDataStreamName to keep behavior consistent with the "return MIN" contract. server/src/main/java/org/opensearch/cluster/metadata/DataStream.java [175-180] static long backingIndexCounterOrMin(String dataStreamName, String indexName) { if (dataStreamName.equals(parseDataStreamName(indexName)) == false) { return Long.MIN_VALUE; } - return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + try { + return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + } catch (NumberFormatException e) { + return Long.MIN_VALUE; + } } Suggestion importance[1-10]: 4 __ Why: A very long digit string is an unlikely input, but wrapping Long.parseLong in try/catch would prevent an unexpected NumberFormatException. Marginal defensive improvement.	Low

Suggestions up to commit 5f3ea85

Category	Suggestion	Impact
Possible issue	Verify wire compatibility across forward versions Because this is a backport to 2.19, the wire-version guard V_2_19_7 will incorrectly attempt to read the new boolean when communicating with any 2.20+/3.x node that does not carry this backport (since those versions are numerically higher than 2.19.7 but do not write the field). Ensure the same backport lands (or use a version check that covers all forward-compatible versions) to avoid stream corruption during mixed-version restores. server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java [183-185] +if (in.getVersion().onOrAfter(Version.V_2_19_7)) { + attachToDataStream = in.readBoolean(); +} - Suggestion importance[1-10]: 6 __ Why: Valid concern about backport wire compatibility—if this change is not present on newer versions (2.20+/3.x), the version guard V_2_19_7 will misread the stream. However, the suggestion is a verification request without a concrete code fix.	Low
General	Surface silent skip on missing stream The restored index is silently skipped when there is no matching pre-existing data stream, even though the user explicitly requested attach_to_data_stream. This produces a confusing outcome where an index is restored as standalone under a .ds-* name while the user expected an attach. Consider logging a warning or failing when attachToDataStream() is set but no target stream exists, so operators are not surprised. server/src/main/java/org/opensearch/snapshots/RestoreService.java [995-1004] for (String restoredIndexName : restoredIndexNames) { String streamName = DataStream.parseDataStreamName(restoredIndexName); if (streamName == null) { continue; } DataStream currentDs = updatedDataStreams.get(streamName); IndexMetadata restoredIndexMetadata = metadata.get(restoredIndexName); - if (currentDs == null || restoredIndexMetadata == null) { + if (currentDs == null) { + logger.warn("attach_to_data_stream set but no data stream [{}] exists for restored index [{}]", streamName, restoredIndexName); + continue; + } + if (restoredIndexMetadata == null) { continue; } Suggestion importance[1-10]: 4 __ Why: Adding a warning log when attach_to_data_stream is set but no stream exists improves observability. It's a minor UX improvement, not a correctness fix.	Low
	Guard against overflow when parsing counter parseDataStreamName already validates the counter portion contains only digits, but Long.parseLong will still throw NumberFormatException if the counter length exceeds long-parseable digits (e.g. very long numeric suffixes). Since parseDataStreamName accepts numeric suffixes "of any length" (per the doc comment referenced in tests), this can crash sorting. Consider guarding the parse or bounding counter length in parseDataStreamName. server/src/main/java/org/opensearch/cluster/metadata/DataStream.java [175-180] static long backingIndexCounterOrMin(String dataStreamName, String indexName) { if (dataStreamName.equals(parseDataStreamName(indexName)) == false) { return Long.MIN_VALUE; } - return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + try { + return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + } catch (NumberFormatException e) { + return Long.MIN_VALUE; + } } Suggestion importance[1-10]: 3 __ Why: parseDataStreamName already validates digits-only, but very long numeric suffixes could theoretically overflow Long.parseLong. The improvement is defensive but low-impact since realistic counters fit in a long.	Low
	Key shared-index check by Index identity Keying indexToStream by index.getName() can miss a shared-Index conflict if the same name maps to different UUIDs across streams, and conversely may false-alarm across recreated indices with same name/different UUID (though the latter is unlikely in practice). Prefer keying by the Index object (name+UUID) to detect true shared backing indices reliably. server/src/main/java/org/opensearch/cluster/metadata/MetadataDataStreamsService.java [158-161] +Map<Index, String> indexToStream = new HashMap<>(); for (DataStream dataStream : finalStreams.values()) { for (Index index : dataStream.getIndices()) { - String previousOwner = indexToStream.putIfAbsent(index.getName(), dataStream.getName()); + String previousOwner = indexToStream.putIfAbsent(index, dataStream.getName()); if (previousOwner != null && previousOwner.equals(dataStream.getName()) == false) { Suggestion importance[1-10]: 2 __ Why: Index names are unique per cluster state, so keying by name is sufficient to detect shared backing indices. The suggested change would actually miss the collision case rather than improve it.	Low

Suggestions up to commit ae988e9

Category	Suggestion	Impact
Possible issue	Hide restored attached backing indices attachRestoredBackingIndices mutates updatedDataStreams via addBackingIndex, but does not update the index's hidden setting like MetadataDataStreamsService.applyAddBackingIndex does. A restored index attached to a data stream will therefore remain non-hidden (unlike every other backing index), which breaks the invariant relied upon elsewhere (e.g., wildcard resolution). Apply the same hide-on-attach behavior here, or route through the shared helper. server/src/main/java/org/opensearch/snapshots/RestoreService.java [634-637] +if (request.attachToDataStream()) { + attachRestoredBackingIndices(indices.keySet(), mdBuilder, updatedDataStreams); +} +mdBuilder.dataStreams(updatedDataStreams); - Suggestion importance[1-10]: 7 __ Why: Valid concern: attachRestoredBackingIndices does not mark the attached index as hidden, unlike MetadataDataStreamsService.applyAddBackingIndex, which breaks the backing-index hidden invariant.	Medium
	Verify cross-version serialization gate correctness Gating serialization on V_2_19_7 means any node on main / 3.x (which does not have this constant on that branch) will not read/write this field and will diverge from 2.19.7+ nodes on the wire. For a backport, the version gate should also include the forward-port version(s) (e.g., the main/3.x version constant) so cross-version communication remains consistent. Verify the corresponding version constant exists on all target branches and adjust the gate accordingly. server/src/main/java/org/opensearch/action/admin/cluster/snapshots/restore/RestoreSnapshotRequest.java [183-185] +if (in.getVersion().onOrAfter(Version.V_2_19_7)) { + attachToDataStream = in.readBoolean(); +} - Suggestion importance[1-10]: 6 __ Why: Version gate consistency for backport across main/3.x is a valid concern for wire compatibility, but the suggestion only asks to verify without concrete change.	Low
	Guard counter parsing against overflow Long.parseLong can throw NumberFormatException for counters that exceed Long.MAX_VALUE, even though parseDataStreamName accepts any numeric-only suffix. Since the goal is to safely order backing indices and never overflow, cap the value at Long.MAX_VALUE on overflow so a pathological name cannot bring down the cluster-state update. Alternatively, tighten parseDataStreamName to reject suffixes that do not fit in a long. server/src/main/java/org/opensearch/cluster/metadata/DataStream.java [175-180] static long backingIndexCounterOrMin(String dataStreamName, String indexName) { if (dataStreamName.equals(parseDataStreamName(indexName)) == false) { return Long.MIN_VALUE; } - return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + try { + return Long.parseLong(indexName.substring(indexName.lastIndexOf('-') + 1)); + } catch (NumberFormatException e) { + return Long.MAX_VALUE; + } } Suggestion importance[1-10]: 5 __ Why: The parsing could throw NumberFormatException for suffixes exceeding Long.MAX_VALUE, though this is an edge case. The suggestion identifies a valid robustness concern but the practical impact is low.	Low
General	Validate against final builder state validateNoSharedBackingIndices is called with currentState.metadata().dataStreams() as the "current" map, but that map still contains the pre-update versions of streams that were mutated. Since the code then putAll(updatedStreams) inside the validator, the effective merge is correct only if every mutated stream appears in updatedStreams; however, other unmutated streams keep their original membership. This is fine for adds, but if an index was moved out of stream A (removed) and added to B in the same request, A's updated (index-free) version overwrites current A correctly — good. Still, consider building the check off metadataBuilder.dataStreams() to guarantee the validation reflects exactly the state being published. server/src/main/java/org/opensearch/cluster/metadata/MetadataDataStreamsService.java [141-145] for (DataStream dataStream : updated.values()) { logger.info("updating data stream [{}]", dataStream.getName()); metadataBuilder.put(dataStream); } -validateNoSharedBackingIndices(currentState.metadata().dataStreams(), updated); +validateNoSharedBackingIndices(metadataBuilder.dataStreams(), java.util.Collections.emptyMap()); Suggestion importance[1-10]: 4 __ Why: The suggestion is speculative; the current validation logic appears correct as it merges updated streams over current ones. The improvement is stylistic rather than fixing a real bug.	Low

[github-actions]

❌ Gradle check result for ae988e9: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 5f3ea85

[github-actions]

❌ Gradle check result for 5f3ea85: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 698afcd

[github-actions]

❌ Gradle check result for 698afcd: null

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

[github-actions]

Persistent review updated to latest commit 698afcd

[github-actions]

❕ Gradle check result for 698afcd: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

[codecov]

Codecov Report

❌ Patch coverage is 97.87986% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.29%. Comparing base (aff3489) to head (fa3f261).
⚠️ Report is 27 commits behind head on 2.19.

Files with missing lines	Patch %	Lines
...h/cluster/metadata/MetadataDataStreamsService.java	98.11%	1 Missing and 1 partial ⚠️
.../java/org/opensearch/snapshots/RestoreService.java	87.50%	1 Missing and 1 partial ⚠️
...ster/snapshots/restore/RestoreSnapshotRequest.java	92.30%	0 Missing and 1 partial ⚠️
...in/indices/datastream/ModifyDataStreamsAction.java	97.05%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               2.19   #22849      +/-   ##
============================================
+ Coverage     71.92%   72.29%   +0.36%     
+ Complexity    66009    64698    -1311     
============================================
  Files          5342     5107     -235     
  Lines        307392   300124    -7268     
  Branches      44862    44119     -743     
============================================
- Hits         221105   216963    -4142     
+ Misses        67823    65037    -2786     
+ Partials      18464    18124     -340     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[github-actions]

Persistent review updated to latest commit fa3f261

[github-actions]

✅ Gradle check result for fa3f261: SUCCESS