Fix breaking changes by delegating and deprecating

[msfroh]

Description

We had a couple of attempts to address the breaking changes detector by restoring old methods and throwing a runtime exception. Instead of maintaining backward compatibility with existing APIs, this actually straight-up broke those APIs.

Related Issues

N/A

Check List

• Functionality includes testing.
• API changes companion pull request created, if applicable.
• Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 979b4ae)

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: Delegate deprecated IndexModule.newIndexService overloads instead of throwing Relevant files: server/src/main/java/org/opensearch/index/IndexModule.java Sub-PR theme: Guard null clusterMergeSchedulerConfig in IndexService and add legacy constructor tests Relevant files: server/src/main/java/org/opensearch/index/IndexService.java server/src/test/java/org/opensearch/index/IndexModuleTests.java

⚡ Recommended focus areas for review Null Defaults The deprecated overload without replicator/segmentReplicationStatsProvider passes s -> {} and s -> null as defaults. These lambda defaults should be validated to ensure they are safe no-ops for all callers, particularly that a null return from segmentReplicationStatsProvider does not cause NullPointerExceptions downstream. s -> {}, s -> null, clusterDefaultMaxMergeAtOnceSupplier ); } Null Config Skipped When clusterMergeSchedulerConfig is null (passed from the deprecated overload), the merge scheduler settings (setDefaultMaxThreadAndMergeCount, setDefaultAutoThrottleEnabled) are silently skipped. This may leave index settings in an unexpected state if callers relied on these being initialized. Verify that skipping these settings is safe and intentional for all legacy callers. if (clusterMergeSchedulerConfig != null) { indexSettings.setDefaultMaxThreadAndMergeCount( clusterMergeSchedulerConfig.getClusterMaxThreadCount(), clusterMergeSchedulerConfig.getClusterMaxMergeCount() ); indexSettings.setDefaultAutoThrottleEnabled(clusterMergeSchedulerConfig.getClusterMergeAutoThrottleEnabled()); } Missing Deprecation Warning The deprecated overloads are annotated with @Deprecated(forRemoval = true) but do not emit a deprecation log message via DEPRECATION_LOGGER. Callers using these methods at runtime will not receive any warning in logs, making it harder to detect and migrate away from deprecated usage. @Deprecated(forRemoval = true) public IndexService newIndexService( IndexService.IndexCreationContext indexCreationContext, NodeEnvironment environment, NamedXContentRegistry xContentRegistry, IndexService.ShardStoreDeleter shardStoreDeleter, CircuitBreakerService circuitBreakerService, BigArrays bigArrays, ThreadPool threadPool, ScriptService scriptService, ClusterService clusterService, Client client, IndicesQueryCache indicesQueryCache, MapperRegistry mapperRegistry, IndicesFieldDataCache indicesFieldDataCache, NamedWriteableRegistry namedWriteableRegistry, BooleanSupplier idFieldDataEnabled, ValuesSourceRegistry valuesSourceRegistry, IndexStorePlugin.DirectoryFactory remoteDirectoryFactory, BiFunction<IndexSettings, ShardRouting, TranslogFactory> translogFactorySupplier, Supplier<TimeValue> clusterDefaultRefreshIntervalSupplier, Supplier<Boolean> fixedRefreshIntervalSchedulingEnabled, Supplier<Boolean> shardLevelRefreshEnabled, RecoverySettings recoverySettings, RemoteStoreSettings remoteStoreSettings, Supplier<Integer> clusterDefaultMaxMergeAtOnceSupplier ) throws IOException { return newIndexService( indexCreationContext, environment, xContentRegistry, shardStoreDeleter, circuitBreakerService, bigArrays, threadPool, scriptService, clusterService, client, indicesQueryCache, mapperRegistry, indicesFieldDataCache, namedWriteableRegistry, idFieldDataEnabled, valuesSourceRegistry, remoteDirectoryFactory, translogFactorySupplier, clusterDefaultRefreshIntervalSupplier, fixedRefreshIntervalSchedulingEnabled, shardLevelRefreshEnabled, recoverySettings, remoteStoreSettings, s -> {}, s -> null, clusterDefaultMaxMergeAtOnceSupplier ); } @Deprecated(forRemoval = true)

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to 979b4ae
Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Apply defaults when config is null When clusterMergeSchedulerConfig is null, the merge scheduler settings are silently skipped, leaving indexSettings with potentially uninitialized or stale defaults. This could lead to incorrect merge behavior for indexes created via the deprecated constructors. Consider applying safe default values when clusterMergeSchedulerConfig is null. server/src/main/java/org/opensearch/index/IndexService.java [360-366] if (clusterMergeSchedulerConfig != null) { indexSettings.setDefaultMaxThreadAndMergeCount( clusterMergeSchedulerConfig.getClusterMaxThreadCount(), clusterMergeSchedulerConfig.getClusterMaxMergeCount() ); indexSettings.setDefaultAutoThrottleEnabled(clusterMergeSchedulerConfig.getClusterMergeAutoThrottleEnabled()); +} else { + // Apply safe defaults when no cluster merge scheduler config is provided (legacy path) + indexSettings.setDefaultMaxThreadAndMergeCount( + MergeSchedulerConfig.DEFAULT_MAX_THREAD_COUNT, + MergeSchedulerConfig.DEFAULT_MAX_MERGE_COUNT + ); + indexSettings.setDefaultAutoThrottleEnabled(MergeSchedulerConfig.DEFAULT_AUTO_THROTTLE); } Suggestion importance[1-10]: 5 __ Why: When clusterMergeSchedulerConfig is null (passed via legacy deprecated constructors), merge scheduler settings are silently skipped. Applying safe defaults in the else branch could prevent unexpected behavior, though the existing indexSettings likely already has built-in defaults from initialization.	Low
General	Document intent of default lambda values The first deprecated newIndexService overload delegates with s -> {} for replicator and s -> null for segmentReplicationStatsProvider, but these are silent no-ops that may hide missing functionality. Verify that these default values are intentional and won't cause null pointer exceptions or silent failures downstream when the results of segmentReplicationStatsProvider are used. server/src/main/java/org/opensearch/index/IndexModule.java [683-710] return newIndexService( - ... - s -> {}, - s -> null, + indexCreationContext, + environment, + xContentRegistry, + shardStoreDeleter, + circuitBreakerService, + bigArrays, + threadPool, + scriptService, + clusterService, + client, + indicesQueryCache, + mapperRegistry, + indicesFieldDataCache, + namedWriteableRegistry, + idFieldDataEnabled, + valuesSourceRegistry, + remoteDirectoryFactory, + translogFactorySupplier, + clusterDefaultRefreshIntervalSupplier, + fixedRefreshIntervalSchedulingEnabled, + shardLevelRefreshEnabled, + recoverySettings, + remoteStoreSettings, + shard -> { /* no-op replicator: intentional for legacy callers */ }, + shardId -> null, /* no segmentation replication stats: intentional for legacy callers */ clusterDefaultMaxMergeAtOnceSupplier ); Suggestion importance[1-10]: 2 __ Why: The suggestion asks to add comments to s -> {} and s -> null lambdas to document intent. While adding clarity is useful, this is essentially a documentation/comment suggestion with minimal functional impact. The improved_code only adds comments without changing logic, making this a low-impact suggestion.	Low

---

Previous suggestions

Suggestions up to commit afc5502

Category	Suggestion	Impact
Possible issue	Replace null with safe default supplier Passing null as the compositeEngineFactorySupplier argument may cause a NullPointerException in the delegated method if it doesn't handle a null supplier. Consider passing a supplier that returns null (e.g., () -> null) or an appropriate default value to make the intent explicit and safe. server/src/main/java/org/opensearch/index/IndexModule.java [766-770] return newIndexService( ... replicator, segmentReplicationStatsProvider, clusterDefaultMaxMergeAtOnceSupplier, - null + () -> null ); Suggestion importance[1-10]: 5 __ Why: Passing null as compositeEngineFactorySupplier could cause NPEs if the receiving method doesn't null-check it. Using () -> null makes the intent explicit and is safer, though the actual impact depends on how the delegated method handles this parameter.	Low
	Avoid returning null from default lambda The lambda s -> null is passed as the segmentReplicationStatsProvider parameter, which returns null for any ShardId. This could cause NullPointerException in callers that don't null-check the result. Consider returning an empty/default ReplicationStats object instead of null. server/src/main/java/org/opensearch/index/IndexModule.java [707-710] return newIndexService( ... s -> {}, - s -> null, + s -> ReplicationStats.empty(), clusterDefaultMaxMergeAtOnceSupplier ); Suggestion importance[1-10]: 4 __ Why: The suggestion is valid in principle - returning null from s -> null could cause NPEs in callers. However, ReplicationStats.empty() may not exist as a method, and the suggestion doesn't verify this against the codebase. The risk depends on how the delegated method handles this parameter.	Low

[github-actions]

❌ Gradle check result for afc5502: 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 979b4ae

[github-actions]

❕ Gradle check result for 979b4ae: 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 80.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.22%. Comparing base (056aac1) to head (979b4ae).
⚠️ Report is 8 commits behind head on main.

Files with missing lines	Patch %	Lines
...rc/main/java/org/opensearch/index/IndexModule.java	60.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #21194      +/-   ##
============================================
+ Coverage     73.18%   73.22%   +0.04%     
+ Complexity    72934    72910      -24     
============================================
  Files          5885     5885              
  Lines        333174   333177       +3     
  Branches      48065    48066       +1     
============================================
+ Hits         243823   243978     +155     
+ Misses        69854    69651     -203     
- Partials      19497    19548      +51     

☔ View full report in Codecov by Sentry.
📢 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.