feat(datastream): add Modify Data Streams API to add/remove backing indices

[madhankb]

Intent

Add a new Modify Data Streams API to OpenSearch: a POST /_data_stream/_modify endpoint that adds or removes backing indices across one or more data streams in a single atomic cluster-state update. New DataStreamAction models an add/remove-backing-index op (PublicApi since 3.8.0). ModifyDataStreamsAction provides the ActionType, Request, and TransportClusterManagerNodeAction applying changes via a ClusterStateUpdateTask; MODIFY_DATA_STREAM cluster-manager task registered. DataStream gains addBackingIndex (inserts at head so write index preserved) and removeBackingIndex throws when index is absent or is the write index. DataStreamFieldMapper merge validator allows enabling _data_stream_timestamp on a mapping merge but never disabling it once enabled. Client wired via IndicesAdminClient/AbstractClient with a CompletionStage async variant. Includes unit tests, an internal cluster IT, and rest-api-spec JSON + YAML tests.

This branch already incorporates: (1) four review fixes the user approved earlier (null-safe data stream lookup avoiding NPE; MapperService closed in finally to avoid analyzer leak; per-action data_stream/index marked required; addBackingIndex rejects an index already belonging to a DIFFERENT data stream); and (2) a fix for the gradle-check failure - RestHighLevelClientTests.testApiNamingConventions failed because the new indices.modify_data_stream rest-api-spec was neither implemented in the High-Level REST Client (deprecated, not extended for new APIs) nor allowlisted. The fix adds indices.modify_data_stream to the notRequiredApi allowlist, consistent with other server-only APIs like wlm_stats_list. Verified locally: :client:rest-high-level:test --tests RestHighLevelClientTests.testApiNamingConventions now passes. This is the only known CI failure (the sibling check-result job just mirrors gradle-check's result). Excluded local artifacts: .osd-toolchain/ and modify-data-stream-test-guide.html.

What Changed

• Adds a POST /_data_stream/_modify endpoint that adds or removes backing indices across one or more data streams in a single atomic cluster-state update, via a new DataStreamAction op model, ModifyDataStreamsAction (ActionType, Request, and TransportClusterManagerNodeAction applying changes through a ClusterStateUpdateTask), RestModifyDataStreamsAction, and a registered MODIFY_DATA_STREAM cluster-manager task.
• Extends DataStream with addBackingIndex (inserts at the head so the write index is preserved, and rejects an index already belonging to a different stream or whose name conflicts with the stream generation) and removeBackingIndex (rejects an absent index or the write index); relaxes the DataStreamFieldMapper merge validator to allow enabling _data_stream_timestamp on a mapping merge but never disabling it once enabled.
• Wires the client through IndicesAdminClient/AbstractClient (with a CompletionStage async variant), adds the indices.modify_data_stream rest-api-spec JSON (allowlisted in RestHighLevelClientTests) plus YAML REST tests, and adds unit tests, request tests, and an internal cluster IT.

Risk Assessment

✅ Low: The two substantive issues from the prior round are correctly fixed with dedicated tests, the residual cross-stream generation-conflict case is provably unreachable, and the only remaining note is an intentional, upstream-consistent behavior change.

Testing

Exercised the new POST /_data_stream/_modify API end-to-end against a real OpenSearch node built from the target commit, capturing a live curl/HTTP transcript that shows add, remove, atomic add+remove, standalone-index adoption, auto hide/un-hide, generation preservation, and every error guard returning HTTP 400 with the correct message (including the generation-conflict and cross-stream review fixes). All supporting automated layers also pass: 27 datastream unit tests, the 3-scenario internal cluster IT, the 3-scenario rest-api-spec YAML test against a real cluster, and the HLRC naming-conventions test that was the previously-known gradle-check failure. No code or test changes were needed; the working tree is clean and the test node was shut down.

Evidence: Live HTTP transcript of POST /_data_stream/_modify against a real OpenSearch node

===============================================================
 Modify Data Streams API — live HTTP transcript
 Endpoint: POST /_data_stream/_modify
 Node: OpenSearch 3.8.0-SNAPSHOT (build "build_hash" : "c475eb827fe6b4d56d9608018b403ca587f963d3")
===============================================================

###############################################################
# 1) Create a data-stream index template for logs-*
###############################################################
$ curl -s -X PUT 'localhost:9200/_index_template/logs-template' -H 'Content-Type: application/json' -d '{"index_patterns":["logs-*"],"data_stream":{},"template":{"settings":{"number_of_shards":1,"number_of_replicas":0}}}'
--- response ---
{"acknowledged":true}

###############################################################
# 2) Create data stream 'logs-app'
###############################################################
$ curl -s -X PUT 'localhost:9200/_data_stream/logs-app'
--- response ---
{"acknowledged":true}

###############################################################
# 3) Roll over twice so there are 3 backing indices (only the last is the write index)
###############################################################
$ curl -s -X POST 'localhost:9200/logs-app/_rollover' >/dev/null; curl -s -X POST 'localhost:9200/logs-app/_rollover' >/dev/null; echo rolled-over
--- response ---
rolled-over


###############################################################
# 4) Inspect backing indices BEFORE modify
###############################################################
$ curl -s 'localhost:9200/_data_stream/logs-app' | python3 -m json.tool
--- response ---
{
    "data_streams": [
        {
            "name": "logs-app",
            "timestamp_field": {
                "name": "@timestamp"
            },
            "indices": [
                {
                    "index_name": ".ds-logs-app-000001",
                    "index_uuid": "o-7AzlKNSw6bh18U_xtQeA"
                },
                {
                    "index_name": ".ds-logs-app-000002",
                    "index_uuid": "lM9a5KRmSC-Nr4wOLtQC4g"
                },
                {
                    "index_name": ".ds-logs-app-000003",
                    "index_uuid": "wVw6wKabS2Kd1YUjy0bTrA"
                }
            ],
            "generation": 3,
            "status": "GREEN",
            "template": "logs-template"
        }
    ]
}


###############################################################
# 5) MODIFY: remove a non-write backing index (.ds-logs-app-000001)
###############################################################
$ curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"remove_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000001"}}]}'
--- response ---
{"acknowledged":true}

###############################################################
# 6) Verify .ds-logs-app-000001 was removed from the data stream (2 indices, generation unchanged)
###############################################################
$ curl -s 'localhost:9200/_data_stream/logs-app' | python3 -c 'import sys,json; d=json.load(sys.stdin)["data_streams"][0]; print("indices:", [i["index_name"] for i in d["indices"]]); print("generation:", d["generation"])'
--- response ---
indices: ['.ds-logs-app-000002', '.ds-logs-app-000003']
generation: 3


###############################################################
# 7) Verify removed index is now a standalone, NON-hidden index again
###############################################################
$ curl -s 'localhost:9200/.ds-logs-app-000001/_settings?flat_settings=true' | python3 -c 'import sys,json; s=json.load(sys.stdin)[".ds-logs-app-000001"]["settings"]; print("index.hidden =", s.get("index.hidden"))'
--- response ---
index.hidden = false


###############################################################
# 8) MODIFY: add it back as a backing index (atomic add)
###############################################################
$ curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000001"}}]}'
--- response ---
{"acknowledged":true}

###############################################################
# 9) Verify it is a backing index again AND re-hidden (auto-adapted)
###############################################################
$ curl -s 'localhost:9200/_data_stream/logs-app' | python3 -c 'import sys,json; d=json.load(sys.stdin)["data_streams"][0]; print("indices:", [i["index_name"] for i in d["indices"]])'; curl -s 'localhost:9200/.ds-logs-app-000001/_settings?flat_settings=true' | python3 -c 'import sys,json; s=json.load(sys.stdin)[".ds-logs-app-000001"]["settings"]; print("index.hidden =", s.get("index.hidden"))'
--- response ---
indices: ['.ds-logs-app-000001', '.ds-logs-app-000002', '.ds-logs-app-000003']
index.hidden = true


###############################################################
# 10) Adopt an EXTERNALLY-created standalone index as a backing index
###############################################################
$ curl -s -X PUT 'localhost:9200/restored-001' -H 'Content-Type: application/json' -d '{"mappings":{"properties":{"@timestamp":{"type":"date"}}}}' >/dev/null; curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":"restored-001"}}]}'; echo; curl -s 'localhost:9200/_data_stream/logs-app' | python3 -c 'import sys,json; d=json.load(sys.stdin)["data_streams"][0]; print("indices:", [i["index_name"] for i in d["indices"]])'
--- response (HTTP status shown) ---
{"acknowledged":true}
indices: ['restored-001', '.ds-logs-app-000001', '.ds-logs-app-000002', '.ds-logs-app-000003']


###############################################################
# 11) ATOMIC multi-action: add restored-002 AND remove .ds-logs-app-000002 in one request
###############################################################
$ curl -s -X PUT 'localhost:9200/restored-002' -H 'Content-Type: application/json' -d '{"mappings":{"properties":{"@timestamp":{"type":"date"}}}}' >/dev/null; curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":"restored-002"}},{"remove_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000002"}}]}'; echo; curl -s 'localhost:9200/_data_stream/logs-app' | python3 -c 'import sys,json; d=json.load(sys.stdin)["data_streams"][0]; print("indices:", [i["index_name"] for i in d["indices"]])'
--- response (HTTP status shown) ---
{"acknowledged":true}
indices: ['restored-002', 'restored-001', '.ds-logs-app-000001', '.ds-logs-app-000003']


###############################################################
# 12) ERROR: removing the WRITE index is rejected (HTTP 400)
###############################################################
$ curl -s -o /dev/null -w 'HTTP %{http_code}
' -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"remove_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000003"}}]}'; curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"remove_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000003"}}]}' | python3 -c 'import sys,json; print("error:", json.load(sys.stdin)["error"]["reason"])'
--- response (HTTP status shown) ---
HTTP 400
error: cannot remove backing index [.ds-logs-app-000003] of data stream [logs-app] because it is the write index


###############################################################
# 13) ERROR: empty actions array is rejected (HTTP 400)
###############################################################
$ curl -s -o /dev/null -w 'HTTP %{http_code}
' -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[]}'; curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[]}' | python3 -c 'import sys,json; print("error:", json.load(sys.stdin)["error"]["reason"])'
--- response (HTTP status shown) ---
HTTP 400
error: Validation Failed: 1: must specify at least one data stream modification action;


###############################################################
# 14) ERROR: index whose name conflicts with a FUTURE backing index is rejected (HTTP 400) [review fix bd3f8dd]
###############################################################
$ curl -s -X PUT 'localhost:9200/.ds-logs-app-000099' -H 'Content-Type: application/json' -d '{"mappings":{"properties":{"@timestamp":{"type":"date"}}}}' >/dev/null; curl -s -o /dev/null -w 'HTTP %{http_code}
' -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000099"}}]}'; curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000099"}}]}' | python3 -c 'import sys,json; print("error:", json.load(sys.stdin)["error"]["reason"])'
--- response (HTTP status shown) ---
HTTP 400
error: cannot add index [.ds-logs-app-000099] to data stream [logs-app] because its name conflicts with a future backing index of the data stream (current generation [3])


###############################################################
# 15) ERROR: adding an index that already belongs to a DIFFERENT data stream is rejected (HTTP 400) [review fix]
###############################################################
$ curl -s -X PUT 'localhost:9200/_data_stream/logs-other' >/dev/null; curl -s -o /dev/null -w 'HTTP %{http_code}
' -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":".ds-logs-other-000001"}}]}'; curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":".ds-logs-other-000001"}}]}' | python3 -c 'import sys,json; print("error:", json.load(sys.stdin)["error"]["reason"])'
--- response (HTTP status shown) ---
HTTP 400
error: index [.ds-logs-other-000001] is already a backing index of data stream [logs-other] and cannot be added to data stream [logs-app]


###############################################################
# 16) ERROR: adding an index with aliases is rejected (HTTP 400)
###############################################################
$ curl -s -X PUT 'localhost:9200/aliased-idx' -H 'Content-Type: application/json' -d '{"aliases":{"my-alias":{}}}' >/dev/null; curl -s -o /dev/null -w 'HTTP %{http_code}
' -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":"aliased-idx"}}]}'; curl -s -X POST 'localhost:9200/_data_stream/_modify' -H 'Content-Type: application/json' -d '{"actions":[{"add_backing_index":{"data_stream":"logs-app","index":"aliased-idx"}}]}' | python3 -c 'import sys,json; print("error:", json.load(sys.stdin)["error"]["reason"])'
--- response (HTTP status shown) ---
HTTP 400
error: cannot add index [aliased-idx] to data stream [logs-app] because it has aliases [my-alias]


===============================================================
 End of transcript. All operations behaved as designed.
===============================================================

Evidence: Core add/remove behavior shown live over HTTP

# remove non-write backing index
POST /_data_stream/_modify {"actions":[{"remove_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000001"}}]}
-> {"acknowledged":true}
indices: ['.ds-logs-app-000002', '.ds-logs-app-000003'] generation: 3 (unchanged)
removed index .ds-logs-app-000001 -> index.hidden = false (standalone again)

# add it back (auto-adapted, inserted at head so write index preserved)
POST /_data_stream/_modify {"actions":[{"add_backing_index":{"data_stream":"logs-app","index":".ds-logs-app-000001"}}]}
-> {"acknowledged":true}
indices: ['.ds-logs-app-000001', '.ds-logs-app-000002', '.ds-logs-app-000003']
.ds-logs-app-000001 -> index.hidden = true (re-hidden)

# atomic add+remove in one request
POST /_data_stream/_modify {"actions":[{"add_backing_index":...restored-002},{"remove_backing_index":...000002}]}
-> {"acknowledged":true}
indices: ['restored-002', 'restored-001', '.ds-logs-app-000001', '.ds-logs-app-000003']

Evidence: Error/validation guards returning HTTP 400 (incl. review fixes)

remove write index: HTTP 400 cannot remove backing index [.ds-logs-app-000003] of data stream [logs-app] because it is the write index
empty actions: HTTP 400 Validation Failed: 1: must specify at least one data stream modification action;
generation conflict (bd3f8dd): HTTP 400 cannot add index [.ds-logs-app-000099] to data stream [logs-app] because its name conflicts with a future backing index of the data stream (current generation [3])
cross-stream index: HTTP 400 index [.ds-logs-other-000001] is already a backing index of data stream [logs-other] and cannot be added to data stream [logs-app]
aliased index: HTTP 400 cannot add index [aliased-idx] to data stream [logs-app] because it has aliases [my-alias]

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 1 info

• ⚠️ server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java:355 - Adding a standalone index whose name matches the backing-index pattern with a counter greater than the data stream's generation (e.g. add .ds-foo-000099 to data stream foo at generation 2) causes Metadata.Builder.build() -> validateDataStreams to throw IllegalStateException. Thrown from inside ClusterStateUpdateTask.execute(), this surfaces to the client as HTTP 500 rather than a 400 bad_request, unlike the other add-index validation failures which throw IllegalArgumentException. Consider validating the candidate index name against the stream generation in addBackingIndex and throwing IllegalArgumentException.
• ℹ️ server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java:267 - The timeout (ack) parameter is parsed in RestModifyDataStreamsAction and advertised in the rest-api-spec JSON, but the transport action uses a plain ClusterStateUpdateTask and unconditionally returns new AcknowledgedResponse(true) in clusterStateProcessed without waiting for node acknowledgements within timeout. The acknowledgement is therefore always true and timeout has no effect (weaker semantics than AckedClusterStateUpdateTask used by sibling metadata-write APIs). This mirrors upstream behavior, so likely intentional — flagging that the documented timeout param is a no-op.
• ℹ️ server/src/main/java/org/opensearch/index/mapper/DataStreamFieldMapper.java:54 - The merge validator (previous, current) -> previous == current || (previous == false && current) compares two boxed Boolean values with == (reference equality) in the previous == current term. It happens to work because of Boolean autobox caching and the second clause handles the false->true case, and the disable-after-enable logic is correct, but reference comparison of boxed types is a fragile pattern. Use value comparison (e.g. previous.equals(current) or Objects.equals).

🔧 Fix: Reject generation-conflicting backing index name; use value equality in mapper
1 info still open:

• ℹ️ server/src/main/java/org/opensearch/index/mapper/DataStreamFieldMapper.java:55 - The new merge validator (previous, current) -> Objects.equals(previous, current) || (previous == false && current) changes the global merge behavior of the _data_stream_timestamp meta field for ALL indices, not just those adopted via the new modify API. Previously (default validator with updateable=false) enabled could not change on any mapping merge; now a MAPPING_UPDATE can flip it false->true on any index. This is required for the adopt-an-index path and matches upstream Elasticsearch behavior, but it also means a user could enable _data_stream_timestamp on a regular (non-data-stream) index via PUT _mapping, which would then require every document to carry a single-valued timestamp field. Flagging as an intentional, upstream-consistent widening of merge semantics for awareness; no action needed.

✅ **Test** - passed

✅ No issues found.

• ./gradlew :server:test --tests org.opensearch.action.admin.indices.datastream.* — 27 unit tests pass (ModifyDataStreamsActionTests=16, ModifyDataStreamsRequestTests=5, DataStreamActionTests=6)
• ./gradlew :rest-api-spec:yamlRestTest --tests org.opensearch.test.rest.ClientYamlTestSuiteIT -Dtests.method="*modify_data_stream*" — 3 YAML REST scenarios pass against a real cluster (remove+add, reject write-index removal, reject empty actions)
• ./gradlew :server:internalClusterTest --tests org.opensearch.action.admin.indices.datastream.ModifyDataStreamsIT — 3 IT scenarios pass (testRemoveThenAddBackingIndex, testAddStandaloneIndexAsBackingIndex, testRemoveWriteIndexFails)
• ./gradlew :client:rest-high-level:test --tests org.opensearch.client.RestHighLevelClientTests.testApiNamingConventions — passes (prior gradle-check failure now green)
• Manual live HTTP transcript via ./gradlew :run + curl against localhost:9200: PUT data-stream template, create data stream, 2x rollover, then POST /_data_stream/_modify for remove / add / adopt-standalone / atomic add+remove and 5 rejected error cases (write-index, empty-actions, cross-stream, aliases, generation-conflict)

✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit da4fbd6)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Potential Index Lookup Issue indices() returns the raw index names from the actions, but IndicesRequest is typically used by the resolver with indicesOptions() to resolve wildcards/aliases and apply security filtering. Since allowAliasesToMultipleIndices=false and the returned names may include indices that are aliases of others or no longer exist, the resolution may interact unexpectedly with the security layer, potentially allowing or blocking requests on indices the user did not intend. Worth verifying that security plugins correctly authorize per-index access for both add and remove operations. public String[] indices() { // resolve to the backing indices being added/removed (not the data streams). This allows the API to repair a // broken data stream definition by referencing indices that may no longer be members of any data stream, and // a DataStreamAction does not support wildcards. return actions.stream().map(DataStreamAction::getIndex).toArray(String[]::new); } Resource Management on Exception In prepareBackingIndex, if mapperServiceFactory.apply(hiddenIndex) throws, that is fine, but if any step after the assignment throws (e.g., mapperService.merge or validateTimestampFieldMapping), the finally block closes the MapperService correctly. However, the cluster state update task runs on the cluster-manager thread (ThreadPool.Names.SAME); creating a temporary MapperService per add action inside a cluster state update may be expensive and could delay the cluster-manager. Consider whether this work belongs outside the cluster state update task for large batches. private static IndexMetadata prepareBackingIndex( IndexMetadata index, String timestampFieldName, CheckedFunction<IndexMetadata, MapperService, IOException> mapperServiceFactory ) throws IOException { final IndexMetadata hiddenIndex = IndexMetadata.builder(index) .settings(Settings.builder().put(index.getSettings()).put(SETTING_INDEX_HIDDEN, true)) .build(); final MapperService mapperService = mapperServiceFactory.apply(hiddenIndex); try { if (hiddenIndex.mapping() != null) { mapperService.merge(hiddenIndex, MapperService.MergeReason.MAPPING_RECOVERY); } mapperService.merge( MapperService.SINGLE_MAPPING_NAME, new ComposableIndexTemplate.DataStreamTemplate(new DataStream.TimestampField(timestampFieldName)) .getDataStreamMappingSnippet(), MapperService.MergeReason.MAPPING_UPDATE ); MetadataCreateDataStreamService.validateTimestampFieldMapping(mapperService); final IndexMetadata.Builder updated = IndexMetadata.builder(hiddenIndex); // Only bump the settings version if hiding the index actually changed its settings; re-adding an index that was // already hidden leaves the settings unchanged and the version must not move (IndexService.updateMetadata asserts this). if (index.getSettings().equals(hiddenIndex.getSettings()) == false) { updated.settingsVersion(1 + hiddenIndex.getSettingsVersion()); } // Likewise, only update the mapping (and bump the mapping version) if merging the _data_stream_timestamp meta field // actually changed the mapping; re-adding an index that was already a backing index leaves the mapping unchanged // (MapperService.updateMapping asserts that a mapping version bump implies a mapping change). final MappingMetadata mergedMapping = new MappingMetadata(mapperService.documentMapper().mappingSource()); final MappingMetadata currentMapping = hiddenIndex.mapping(); if (currentMapping == null || currentMapping.source().equals(mergedMapping.source()) == false) { updated.putMapping(mergedMapping).mappingVersion(1 + hiddenIndex.getMappingVersion()); } return updated.build(); } finally { IOUtils.close(mapperService); } } Behavior Change The previous dataStream(String) would NPE if no DataStreamMetadata was present in customs; the new version returns null in that case. This is a deliberate fix per the PR description, but callers elsewhere that previously relied on getting an NPE (or assumed metadata exists) may now silently get null. Verify all existing callers of Metadata.Builder.dataStream(name) handle a null return correctly. public DataStream dataStream(String dataStreamName) { return dataStreams().get(dataStreamName); } public Map<String, DataStream> dataStreams() { return Optional.ofNullable((DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE)) .map(DataStreamMetadata::dataStreams) .orElse(Collections.emptyMap()); }

[github-actions]

Persistent review updated to latest commit 58474de

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to da4fbd6

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Deduplicate indices returned for resolution indices() may return duplicate index names if multiple actions reference the same index, which can cause issues during index resolution/authorization. Consider deduplicating the returned array using distinct() to ensure each index is listed only once. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [163-169] @Override public String[] indices() { - // resolve to the backing indices being added/removed (not the data streams). This allows the API to repair a - // broken data stream definition by referencing indices that may no longer be members of any data stream, and - // a DataStreamAction does not support wildcards. - return actions.stream().map(DataStreamAction::getIndex).toArray(String[]::new); + return actions.stream().map(DataStreamAction::getIndex).distinct().toArray(String[]::new); } Suggestion importance[1-10]: 4 __ Why: Deduplicating indices may be beneficial for authorization and resolution efficiency, but the impact is minor since duplicates are uncommon and typically harmless.	Low
	Guard against null actions list The constructor does not guard against a null actions list, which would cause a NullPointerException in Collections.unmodifiableList and bypass the validate() check. Add a null check (e.g., via Objects.requireNonNull or by treating null as empty) so that a malformed request fails with a clear validation error rather than an NPE. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [136-138] public Request(List<DataStreamAction> actions) { - this.actions = Collections.unmodifiableList(actions); + this.actions = Collections.unmodifiableList(Objects.requireNonNull(actions, "actions must not be null")); } Suggestion importance[1-10]: 3 __ Why: Adding a null check is a minor defensive improvement; the request is constructed via XContent parser which typically doesn't pass null, so the impact is limited.	Low
	Clarify write index detection condition The write-index check should also reject the case where the data stream has only a single backing index (which is also the write index). The current condition indices.size() == backingIndexPosition + 1 correctly covers this, but consider that removing any backing index from a single-index data stream leaves it in an invalid state with no write index. The error message is accurate but the resulting state from removal scenarios on tiny streams should be re-verified. server/src/main/java/org/opensearch/cluster/metadata/DataStream.java [168-177] -if (indices.size() == (backingIndexPosition + 1)) { +if (backingIndexPosition == indices.size() - 1) { throw new IllegalArgumentException( String.format( Locale.ROOT, "cannot remove backing index [%s] of data stream [%s] because it is the write index", index.getName(), name ) ); } Suggestion importance[1-10]: 2 __ Why: The suggestion is essentially a readability tweak; the existing condition is logically equivalent and the rewording does not change behavior.	Low

---

Previous suggestions

Suggestions up to commit f38773a

Category	Suggestion	Impact
Possible issue	Guard against null actions list The constructor will throw a NullPointerException if actions is null (e.g. when the REST body omits the actions field, ConstructingObjectParser may pass null). Guard against null and convert to an empty list so that validate() can produce a proper validation error instead of a NPE during request construction. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [136-138] public Request(List<DataStreamAction> actions) { - this.actions = Collections.unmodifiableList(actions); + this.actions = actions == null ? Collections.emptyList() : Collections.unmodifiableList(actions); } Suggestion importance[1-10]: 3 __ Why: The Request is typically constructed via the parser with constructorArg() which requires the field, or programmatically with a non-null list. A NPE guard is a minor defensive improvement with limited real-world impact.	Low
General	Defensively handle counter parsing IndexMetadata.parseIndexNameCounter may throw on malformed names; even though the prefix and number pattern check guards against most cases, the check on the captured digits (e.g. very large numeric strings) can throw NumberFormatException. Wrap the counter check defensively or rely strictly on the regex group to avoid surfacing an unexpected exception as a 500 to the user. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [320-323] final String backingIndexPrefix = DataStream.BACKING_INDEX_PREFIX + dataStreamName + "-"; if (indexName.startsWith(backingIndexPrefix) - && Metadata.NUMBER_PATTERN.matcher(indexName.substring(backingIndexPrefix.length())).matches() - && IndexMetadata.parseIndexNameCounter(indexName) > dataStream.getGeneration()) { + && Metadata.NUMBER_PATTERN.matcher(indexName.substring(backingIndexPrefix.length())).matches()) { + long counter; + try { + counter = IndexMetadata.parseIndexNameCounter(indexName); + } catch (IllegalArgumentException ex) { + counter = -1; + } + if (counter > dataStream.getGeneration()) { Suggestion importance[1-10]: 3 __ Why: The preceding NUMBER_PATTERN regex check already restricts to numeric suffixes, so parseIndexNameCounter is unlikely to throw. Adds minor defensive value but is mostly redundant.	Low
	Filter nulls from indices array DataStreamAction.getIndex() can return null if the action was constructed without an index (e.g. an invalid stream input). Filter null values out to avoid NullPointerException downstream in IndexNameExpressionResolver and security/auth filters that iterate over indices(). server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [163-169] @Override public String[] indices() { // resolve to the backing indices being added/removed (not the data streams). This allows the API to repair a // broken data stream definition by referencing indices that may no longer be members of any data stream, and // a DataStreamAction does not support wildcards. - return actions.stream().map(DataStreamAction::getIndex).toArray(String[]::new); + return actions.stream().map(DataStreamAction::getIndex).filter(Objects::nonNull).toArray(String[]::new); } Suggestion importance[1-10]: 2 __ Why: DataStreamAction parser requires both data_stream and index fields, so getIndex() should not return null in practice. The suggestion offers marginal defensive value.	Low

Suggestions up to commit bd3f8dd

Category	Suggestion	Impact
General	Honor ack timeout for cluster state update The task is submitted with Priority.URGENT but never honors request.ackTimeout(), so the response is acknowledged as soon as the cluster state is processed locally, without waiting for the change to be acked by other nodes. Use AckedClusterStateUpdateTask (as done by other acknowledged data-stream actions) so the ?timeout= parameter is respected and the acknowledged flag reflects the actual cluster acknowledgment. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [262-269] @Override public ClusterState execute(ClusterState currentState) throws IOException { return modifyDataStream(currentState, request.getActions(), indicesService::createIndexMapperService); } @Override public void clusterStateProcessed(String source, ClusterState oldState, ClusterState newState) { + // Consider switching to AckedClusterStateUpdateTask to honor request.ackTimeout() listener.onResponse(new AcknowledgedResponse(true)); } Suggestion importance[1-10]: 7 __ Why: Valid concern: using a plain ClusterStateUpdateTask rather than AckedClusterStateUpdateTask means the ackTimeout/timeout parameter is not honored, which is a meaningful behavior issue for acknowledged actions.	Medium
	Verify behavior change of dataStream lookup The previous dataStream(...) implementation threw an NPE when no DataStreamMetadata custom was present; the new implementation silently returns null, changing behavior for callers that may have relied on that contract. Verify all existing callers handle a null return correctly when no data streams have ever been created in the cluster. server/src/main/java/org/opensearch/cluster/metadata/Metadata.java [1377-1385] +public DataStream dataStream(String dataStreamName) { + return dataStreams().get(dataStreamName); +} +public Map<String, DataStream> dataStreams() { + return Optional.ofNullable((DataStreamMetadata) this.customs.get(DataStreamMetadata.TYPE)) + .map(DataStreamMetadata::dataStreams) + .orElse(Collections.emptyMap()); +} Suggestion importance[1-10]: 5 __ Why: Valid observation about behavior change from NPE to null return; worth verifying callers, but existing_code equals improved_code and it's only a verification request.	Low
	Guard request constructor against null actions The constructor does not guard against a null actions list, which would cause a NullPointerException from Collections.unmodifiableList and bypass the validate() check that only handles the empty case. Defensively reject null here (or in validate()) to return a proper validation error rather than an NPE. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [136-138] public Request(List<DataStreamAction> actions) { - this.actions = Collections.unmodifiableList(actions); + this.actions = Collections.unmodifiableList(Objects.requireNonNull(actions, "actions must not be null")); } Suggestion importance[1-10]: 4 __ Why: Minor defensive improvement; in practice the request is constructed from XContent or StreamInput which would not produce null, but adding null guard improves robustness.	Low
	Prevent NPE on serializing partial action dataStream and index are declared as non-final and can remain null if the action is constructed via the private DataStreamAction(Type) ctor used by the parsers without the required fields being declared. The writeTo will then NPE on out.writeString(null). Add a null-check in writeTo or enforce non-null in a final validation step after parsing. server/src/main/java/org/opensearch/action/admin/indices/datastream/DataStreamAction.java [133-137] public DataStreamAction(StreamInput in) throws IOException { this.type = Type.fromValue(in.readByte()); this.dataStream = in.readString(); this.index = in.readString(); } +// and in writeTo: +// Objects.requireNonNull(dataStream, "dataStream must be set"); +// Objects.requireNonNull(index, "index must be set"); + Suggestion importance[1-10]: 3 __ Why: The parser already declares required fields, so reaching writeTo with null is unlikely; the suggestion is defensive but low impact.	Low

Suggestions up to commit 58474de

Category	Suggestion	Impact
General	Robustly detect hidden setting change Comparing index.getSettings() to hiddenIndex.getSettings() via equals may not reliably detect the hidden flag change because Settings.equals compares all entries, but the original index settings might have an absent index.hidden while hiddenIndex has it explicitly set to true even when the effective value matches. Compare the resolved INDEX_HIDDEN_SETTING values instead to robustly determine whether the settings actually changed. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [386-390] final IndexMetadata.Builder updated = IndexMetadata.builder(hiddenIndex); // Only bump the settings version if hiding the index actually changed its settings; re-adding an index that was // already hidden leaves the settings unchanged and the version must not move (IndexService.updateMetadata asserts this). -if (index.getSettings().equals(hiddenIndex.getSettings()) == false) { +if (INDEX_HIDDEN_SETTING.get(index.getSettings()) == false) { updated.settingsVersion(1 + hiddenIndex.getSettingsVersion()); } Suggestion importance[1-10]: 6 __ Why: Valid concern: comparing the entire Settings object may not reliably reflect whether index.hidden actually changed (e.g., if it was already explicitly false). Using INDEX_HIDDEN_SETTING.get would more accurately gate the settings version bump, which is asserted on by IndexService.updateMetadata.	Low
	Guard against null serialization fields The constructor reads dataStream and index as non-nullable strings, but the fields can be null when constructed via the package-private DataStreamAction(Type) parser path before required fields are populated. If a malformed or partially populated instance is ever serialized, writeString will throw NPE. Validate that both fields are non-null in writeTo (or use writeOptionalString/readOptionalString) to fail fast with a clear error. server/src/main/java/org/opensearch/action/admin/indices/datastream/DataStreamAction.java [133-137] -public DataStreamAction(StreamInput in) throws IOException { - this.type = Type.fromValue(in.readByte()); - this.dataStream = in.readString(); - this.index = in.readString(); +@Override +public void writeTo(StreamOutput out) throws IOException { + out.writeByte(type.value()); + out.writeString(Objects.requireNonNull(dataStream, "dataStream must not be null")); + out.writeString(Objects.requireNonNull(index, "index must not be null")); } Suggestion importance[1-10]: 3 __ Why: Minor defensive improvement; the parser/factory paths populate the fields before serialization in practice, so the NPE risk is hypothetical. Adding null checks could improve error clarity but has limited real impact.	Low
	Use indices lookup for membership check Iterating all data streams in the cluster on every add operation is O(N) per action and can be costly when many data streams exist. Consider using the cluster's indices lookup (e.g., Metadata.getIndicesLookup) to determine in O(1) whether the candidate index already belongs to another data stream. server/src/main/java/org/opensearch/action/admin/indices/datastream/ModifyDataStreamsAction.java [326-338] -for (DataStream other : builder.dataStreams().values()) { - if (other.getName().equals(dataStreamName) == false && other.getIndices().contains(index.getIndex())) { - throw new IllegalArgumentException( - "index [" - + indexName - + "] is already a backing index of data stream [" - + other.getName() - + "] and cannot be added to data stream [" - + dataStreamName - + "]" - ); - } +IndexAbstraction abstraction = builder.build().getIndicesLookup().get(indexName); +if (abstraction != null && abstraction.getParentDataStream() != null + && abstraction.getParentDataStream().getName().equals(dataStreamName) == false) { + throw new IllegalArgumentException( + "index [" + + indexName + + "] is already a backing index of data stream [" + + abstraction.getParentDataStream().getName() + + "] and cannot be added to data stream [" + + dataStreamName + + "]" + ); } Suggestion importance[1-10]: 3 __ Why: The optimization is reasonable but calling builder.build() to access getIndicesLookup mid-modification is itself expensive and may not be correct/available; the suggested improved_code may not be a clean drop-in. Marginal benefit for typical cluster sizes.	Low

[github-actions]

❌ Gradle check result for 58474de: 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?

[madhankb]

Background:
I'm building a tool that allows customers to take a manual snapshot and seamlessly restore it to a new domain. During development, I discovered that snapshotting and restoring data streams is a significant challenge in OpenSearch today. To address this, I'm leveraging the Modify Data Streams API to temporarily remove backing indices from the data stream, take the snapshot, restore it, and then re-add the backing indices. I've updated the code to include this as a new feature. Could someone review this approach and let me know if it makes sense?

[github-actions]

Persistent review updated to latest commit bd3f8dd

[github-actions]

✅ Gradle check result for bd3f8dd: SUCCESS

[codecov]

Codecov Report

❌ Patch coverage is 92.59259% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.40%. Comparing base (79d8e1e) to head (da4fbd6).
⚠️ Report is 9 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/org/opensearch/cluster/metadata/DataStream.java	91.66%	0 Missing and 1 partial ⚠️
...opensearch/index/mapper/DataStreamFieldMapper.java	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22316      +/-   ##
============================================
+ Coverage     73.38%   73.40%   +0.01%     
- Complexity    76071    76093      +22     
============================================
  Files          6076     6076              
  Lines        345461   345488      +27     
  Branches      49723    49728       +5     
============================================
+ Hits         253527   253597      +70     
+ Misses        71731    71694      -37     
+ Partials      20203    20197       -6     

☔ 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 f38773a

[github-actions]

✅ Gradle check result for f38773a: SUCCESS

[github-actions]

Persistent review updated to latest commit da4fbd6

[github-actions]

✅ Gradle check result for da4fbd6: SUCCESS