Enable integ test case for context aware segments #20457
Conversation
RS146BIJAY
requested review from
a team,
Bukhtawar,
CEHENKLE,
Rishikesh1159,
anasalkouz,
andrross,
ashking94,
cwperks,
dbwiddis,
gbbafna,
jed326,
kotwanikunal,
mch2,
msfroh,
owaiskazi19,
reta,
sachinpkale,
saratvemulapalli,
shwetathareja and
sohami
as code owners
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis PR introduces a context-aware migration experimental feature by adding a Changes
Sequence DiagramsequenceDiagram
participant Client as Test Client
participant Plugin as ContextAwareIndexPlugin
participant Engine as Indexing Engine
participant Listener as ContextAwareIndexOperationListener
participant Index as Index
Client->>Plugin: Initialize cluster with plugin
Plugin->>Plugin: onIndexModule() invoked
Plugin->>Engine: Register ContextAwareIndexOperationListener
Client->>Engine: Submit index request
Engine->>Listener: preIndex(IndexingOperationListener.Context)
Listener->>Listener: Check if context-aware enabled
alt Context-Aware Enabled
Listener->>Index: Set document grouping criteria to "-1"
Listener->>Engine: Return context with modified criteria
else Context-Aware Disabled
Listener->>Engine: Return unmodified context
end
Engine->>Index: Index document with criteria
Engine->>Client: Return success
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/src/main/java/org/opensearch/index/shard/IndexShard.java (1)
4992-5001: Update the comment to reflect current behavior and verify CAS sync correctness with search-idle enabled.The inline comment at line 4995–4996 states that context-aware indices disable search-idle to ensure periodic sync between parent and child IndexWriter, but the implementation (line 5001) does not check for context-aware enablement. This mismatch is misleading. Since CAS indices use CompositeIndexWriter that relies on periodic refreshes to sync writers, confirm that skipping refreshes due to search-idle does not break that sync (e.g., via integration tests).
💡 Suggested comment update
- // TODO: Should we disable this as data will never get in sync if we use search idle for context aware segments. - public final boolean isSearchIdleSupported() { - // If the index is remote store backed, then search idle is not supported. This is to ensure that async - // refresh. If the index is context aware enabled, search idle is not supported. This will ensure periodic sync - // between child and parent IndexWriter. - // task continues to upload to remote store periodically. + // Search-idle is disabled only for remote-store backed indices to ensure async refresh/upload keeps running. + public final boolean isSearchIdleSupported() { + // For remote-store backed indices, search idle is not supported to ensure async refresh/upload keeps running. if (isRemoteTranslogEnabled() || indexSettings.isAssignedOnRemoteNode()) { return false; } return indexSettings.isSegRepEnabledOrRemoteNode() == false || indexSettings.getNumberOfReplicas() == 0; }
🤖 Fix all issues with AI agents
In
`@modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.java`:
- Around line 112-122: The PluginInfo instantiation references
ContextAwareIndexPlugin.class.getName() without an import and uses inconsistent
symbols/values; replace the bare reference with the nested class reference
OpenSearchIntegTestCase.ContextAwareIndexPlugin.class.getName() (or add the
appropriate import for ContextAwareIndexPlugin), change
org.opensearch.Version.CURRENT to the already-used Version.CURRENT for
consistency, and update the plugin version string "1.8" to match the other
plugins (e.g., "21") so PluginInfo, ContextAwareIndexPlugin, and Version.CURRENT
usages compile and are consistent.
In `@server/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.java`:
- Around line 548-554: The loop in CompositeIndexWriter is closing each child
IndexWriter before calling getPendingNumDocs(), which can throw
AlreadyClosedException; change the loop to first obtain a local reference to the
child writer (e.g., IndexWriter iw = childDisposableWriter.getIndexWriter()),
then call iw.getPendingNumDocs() and add to pendingNumDocsByOldChildWriter, then
collect iw.getDirectory() and finally call iw.close(); update uses of
getIndexWriter() in the loop to that local variable (refer to
markForRefreshIndexWritersMap, DisposableIndexWriter, getIndexWriter(),
getPendingNumDocs(), getDirectory(), close()).
In
`@test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java`:
- Around line 157-159: Replace the unresolved ContextAwareIndexPlugin reference
in nodePlugins() by using the fully qualified nested class used elsewhere:
change ContextAwareIndexPlugin.class to
OpenSearchIntegTestCase.ContextAwareIndexPlugin.class (so nodePlugins() returns
List.of(MockRepository.Plugin.class,
OpenSearchIntegTestCase.ContextAwareIndexPlugin.class)); this avoids adding an
import and matches other usages.
🧹 Nitpick comments (2)
server/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.java (1)
348-357: Align timeout-basedtryAcquirewith closed-map handling.
tryAcquire()now unlocks and returnsnullwhen the lookup is closed, buttryAcquire(TimeValue)can still return a closed lookup. Consider mirroring the closed check there to keep semantics consistent and avoid operations on closed maps in timeout code paths.modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.java (1)
116-117: Inconsistent version and redundant qualification.Two minor observations:
- The plugin version
"1.8"differs from other plugins in this file which use"21". Verify if this is intentional.org.opensearch.Version.CURRENTat line 116 is fully qualified, butVersionis already imported at line 11, so you can useVersion.CURRENTfor consistency with line 94.Suggested fix
new PluginInfo( ContextAwareIndexPlugin.class.getName(), "classpath plugin", "NA", - org.opensearch.Version.CURRENT, - "1.8", + Version.CURRENT, + "21", ContextAwareIndexPlugin.class.getName(), null, Collections.emptyList(), false )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.javaserver/src/internalClusterTest/java/org/opensearch/action/admin/cluster/node/tasks/ConcurrentSearchTasksIT.javaserver/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/ShardsLimitAllocationDeciderIT.javaserver/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.javaserver/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.javaserver/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerNoopIT.javaserver/src/internalClusterTest/java/org/opensearch/indices/replication/WarmIndexSegmentReplicationIT.javaserver/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.javaserver/src/internalClusterTest/java/org/opensearch/remotestore/WritableWarmIT.javaserver/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.javaserver/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.javaserver/src/main/java/org/opensearch/index/shard/IndexShard.javaserver/src/test/java/org/opensearch/action/admin/indices/tiering/TransportHotToWarmTieringActionTests.javatest/framework/src/main/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.javatest/framework/src/main/java/org/opensearch/remotestore/RemoteStoreCoreTestCase.javatest/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.javatest/framework/src/main/java/org/opensearch/test/AbstractMultiClustersTestCase.javatest/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.javatest/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-01-13T17:40:27.167Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20411
File: server/src/main/java/org/opensearch/index/codec/CodecService.java:112-133
Timestamp: 2026-01-13T17:40:27.167Z
Learning: Avoid capturing or evaluating a supplier (e.g., this::defaultCodec) upfront when passing it to a registry during object construction. If registries may replace defaults during iteration (as in EnginePlugin.getAdditionalCodecs), pass the supplier itself and only resolve it at use time. This ensures dynamic behavior is preserved during initialization and prevents premature binding of defaults in codecs/registry setup. This pattern should apply to similar initialization paths in Java server code where registries may mutate defaults during construction.
Applied to files:
server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.javaserver/src/main/java/org/opensearch/index/shard/IndexShard.javaserver/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.java
📚 Learning: 2025-12-02T22:44:14.799Z
Learnt from: prudhvigodithi
Repo: opensearch-project/OpenSearch PR: 20112
File: server/src/internalClusterTest/java/org/opensearch/search/slice/SearchSliceIT.java:73-81
Timestamp: 2025-12-02T22:44:14.799Z
Learning: In OpenSearch integration tests extending OpenSearchIntegTestCase, using `LuceneTestCase.SuppressCodecs("*")` triggers special handling that selects a random production codec from the CODECS array, while `SuppressCodecs("Asserting")` or other specific codec suppressions still allow Lucene's default codec randomization which may include the asserting codec. Use `SuppressCodecs("*")` when you need to completely avoid asserting codecs (e.g., for cross-thread StoredFieldsReader usage) while maintaining production codec test coverage.
Applied to files:
server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java
📚 Learning: 2026-01-02T19:23:16.689Z
Learnt from: karenyrx
Repo: opensearch-project/OpenSearch PR: 20335
File: modules/transport-grpc/src/main/java/org/opensearch/transport/grpc/proto/request/search/SearchSourceBuilderProtoUtils.java:155-172
Timestamp: 2026-01-02T19:23:16.689Z
Learning: In OpenSearch's gRPC transport module, treat removals of previously-unsupported fields (those throwing UnsupportedOperationException) from experimental proto schemas as non-breaking changes. When reviewing changes to proto-related Java code in this module, document that such removals do not count as breaking API changes, and ensure tests reflect compatibility expectations accordingly.
Applied to files:
modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.java
📚 Learning: 2025-12-12T13:31:51.234Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:51.234Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java
🧬 Code graph analysis (9)
server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/ShardsLimitAllocationDeciderIT.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
server/src/test/java/org/opensearch/action/admin/indices/tiering/TransportHotToWarmTieringActionTests.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
server/src/internalClusterTest/java/org/opensearch/remotestore/WritableWarmIT.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerNoopIT.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/node/tasks/ConcurrentSearchTasksIT.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java (2)
server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerNoopIT.java (1)
OpenSearchIntegTestCase(52-100)server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java (1)
OpenSearchIntegTestCase(21-86)
server/src/internalClusterTest/java/org/opensearch/indices/replication/WarmIndexSegmentReplicationIT.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java (1)
server/src/main/java/org/opensearch/common/util/FeatureFlags.java (1)
FeatureFlags(29-341)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
- GitHub Check: gradle-check
- GitHub Check: detect-breaking-change
- GitHub Check: Analyze (java)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
🔇 Additional comments (23)
server/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.java (1)
42-48: No issues found.server/src/internalClusterTest/java/org/opensearch/remotestore/WritableWarmIT.java (1)
77-82: LGTM!The addition of
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_FLAGfollows the existing pattern for enabling experimental feature flags in this test class. The import forFeatureFlagsis already present.server/src/main/java/org/opensearch/common/settings/FeatureFlagSettings.java (1)
41-42: LGTM!The addition of
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_SETTINGtoBUILT_IN_FEATURE_FLAGSis required to register the new experimental feature flag with the settings framework. This correctly aligns with the flag definition inFeatureFlags.java.server/src/test/java/org/opensearch/action/admin/indices/tiering/TransportHotToWarmTieringActionTests.java (1)
47-52: LGTM!The addition of
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_FLAGfollows the existing pattern. Note that this test class is currently disabled via@LuceneTestCase.AwaitsFix(bugUrl = "")annotation at line 39.server/src/internalClusterTest/java/org/opensearch/cluster/shards/ClusterShardLimitIT.java (1)
140-145: LGTM!The addition of
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_FLAGcorrectly follows the existing pattern for enabling experimental feature flags in this parameterized test class.server/src/internalClusterTest/java/org/opensearch/cluster/routing/allocation/decider/ShardsLimitAllocationDeciderIT.java (1)
105-110: LGTM!The addition of
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_FLAGcorrectly follows the existing pattern for enabling experimental feature flags in this parameterized test class.server/src/internalClusterTest/java/org/opensearch/index/shard/IndexShardIT.java (2)
98-98: LGTM!The import is correctly added to access the
ContextAwareIndexOperationListenerinner class fromOpenSearchIntegTestCase.
668-669: LGTM!The
ContextAwareIndexOperationListeneris correctly added as a second listener to thenewIndexShardcall. The method signature at line 696 accepts varargs (IndexingOperationListener... listeners), so this correctly passes both the locallistenerand the new context-aware listener to enable the experimental feature during shard recovery tests.server/src/internalClusterTest/java/org/opensearch/indices/memory/breaker/CircuitBreakerNoopIT.java (2)
37-37: LGTM!Import correctly added for
FeatureFlagsto access the context-aware migration flag constant.
63-63: LGTM!The
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_FLAGis correctly enabled innodeSettings(). This aligns with the PR objective to enable context-aware segments for integration tests.server/src/internalClusterTest/java/org/opensearch/indices/replication/WarmIndexSegmentReplicationIT.java (1)
160-160: LGTM!The
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_FLAGis correctly added tofeatureFlagSettings()alongside the existingWRITABLE_WARM_INDEX_EXPERIMENTAL_FLAG. This enables both experimental features for warm index segment replication tests as intended.server/src/internalClusterTest/java/org/opensearch/action/admin/cluster/node/tasks/ConcurrentSearchTasksIT.java (2)
18-18: LGTM!Import correctly added for
FeatureFlags.
71-72: LGTM!The
CONTEXT_AWARE_MIGRATION_EXPERIMENTAL_FLAGis correctly enabled after the built-in feature flags loop. This ensures the flag is explicitly set totruefor concurrent search task tests, regardless of its default value.test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java (4)
137-146: Import-only change — no review needed.Also applies to: 230-230
696-698: Randomized CAS index setting looks good.This keeps coverage for both CAS-enabled and disabled indices via existing randomized settings.
717-717: Base feature flag enablement is clear.Enabling the CAS experimental flag here aligns with the test-wide intent.
2019-2031: ContextAwareIndexPlugin wiring is clean.Listener registration is correctly gated on the index setting, keeping behavior scoped.
Also applies to: 3012-3034
test/framework/src/main/java/org/opensearch/test/OpenSearchSingleNodeTestCase.java (1)
277-279: CAS plugin added to single-node harness looks good.Ensures the listener is available for single-node tests.
server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java (1)
68-79: Classpath plugin list update looks fine.ContextAwareIndexPlugin aligns with other test harness updates.
test/framework/src/main/java/org/opensearch/test/AbstractMultiClustersTestCase.java (1)
80-82: CAS plugin inclusion for multi-cluster nodes looks good.test/framework/src/main/java/org/opensearch/remotestore/RemoteStoreBaseIntegTestCase.java (1)
78-79: Flush count tracking integrates cleanly with existing stats.The new FLUSHED_OPERATIONS metric will help downstream tests adjust expectations.
Also applies to: 105-111, 137-140
test/framework/src/main/java/org/opensearch/remotestore/RemoteStoreCoreTestCase.java (2)
18-18: Import-only change — no review needed.
221-258: CAS-aware metadata file accounting is a good test hardening.Using FLUSHED_OPERATIONS to adjust expectations should reduce CAS-related flakiness.
Also applies to: 276-298, 338-359
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| new PluginInfo( | ||
| ContextAwareIndexPlugin.class.getName(), | ||
| "classpath plugin", | ||
| "NA", | ||
| org.opensearch.Version.CURRENT, | ||
| "1.8", | ||
| ContextAwareIndexPlugin.class.getName(), | ||
| null, | ||
| Collections.emptyList(), | ||
| false | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find ContextAwareIndexPlugin and check its package
fd -t f 'ContextAwareIndexPlugin.java' --exec head -30 {}Repository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
# Check the imports and full context of the test file
cat -n modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.java | head -130Repository: opensearch-project/OpenSearch
Length of output: 5465
🏁 Script executed:
# Search for all PluginInfo entries to compare versions
rg -A 10 "new PluginInfo" modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.javaRepository: opensearch-project/OpenSearch
Length of output: 1130
🏁 Script executed:
# Search for ContextAwareIndexPlugin class definition anywhere in codebase
rg "class ContextAwareIndexPlugin" -A 3Repository: opensearch-project/OpenSearch
Length of output: 529
🏁 Script executed:
# Also search for any reference to ContextAwareIndexPlugin to understand context
rg "ContextAwareIndexPlugin" --type javaRepository: opensearch-project/OpenSearch
Length of output: 1624
🏁 Script executed:
# Check how ContextAwareIndexPlugin is imported/used in ClasspathPluginIT.java
cat -n server/src/internalClusterTest/java/org/opensearch/plugins/ClasspathPluginIT.java | head -150Repository: opensearch-project/OpenSearch
Length of output: 3295
🏁 Script executed:
# Check the OpenSearchIntegTestCase file to understand the class structure
cat -n test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java | grep -A 30 "public static class ContextAwareIndexPlugin"Repository: opensearch-project/OpenSearch
Length of output: 1159
🏁 Script executed:
# Check what LoadExtendingPluginServicesIT extends and if it has access to ContextAwareIndexPlugin
rg -A 5 "class LoadExtendingPluginServicesIT" modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.javaRepository: opensearch-project/OpenSearch
Length of output: 445
🏁 Script executed:
# Check what GrpcTransportBaseIT is and what it extends
fd -t f 'GrpcTransportBaseIT.java' --exec head -50 {}Repository: opensearch-project/OpenSearch
Length of output: 2007
🏁 Script executed:
# Check if LoadExtendingPluginServicesIT imports ContextAwareIndexPlugin or OpenSearchIntegTestCase
cat -n modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.java | grep -E "import|ContextAware|OpenSearchIntegTestCase"Repository: opensearch-project/OpenSearch
Length of output: 1086
🏁 Script executed:
# Check how nested class access works - verify if similar patterns exist in the codebase
rg "OpenSearchIntegTestCase\.ContextAwareIndexPlugin" --type javaRepository: opensearch-project/OpenSearch
Length of output: 412
Add missing import for ContextAwareIndexPlugin or use fully qualified name.
The code references ContextAwareIndexPlugin.class.getName() (lines 113, 118) but does not import it. Although ContextAwareIndexPlugin is a static nested class of OpenSearchIntegTestCase (which GrpcTransportBaseIT extends), the bare reference will not compile without an import. Either import OpenSearchIntegTestCase or use OpenSearchIntegTestCase.ContextAwareIndexPlugin.class.getName().
Additionally, line 116 inconsistently uses org.opensearch.Version.CURRENT instead of the imported Version.CURRENT (used at lines 94 and 105). And the version "1.8" for ContextAwareIndexPlugin differs from "21" used for the other plugins in this same method.
🤖 Prompt for AI Agents
In
`@modules/transport-grpc/src/internalClusterTest/java/org/opensearch/transport/grpc/LoadExtendingPluginServicesIT.java`
around lines 112 - 122, The PluginInfo instantiation references
ContextAwareIndexPlugin.class.getName() without an import and uses inconsistent
symbols/values; replace the bare reference with the nested class reference
OpenSearchIntegTestCase.ContextAwareIndexPlugin.class.getName() (or add the
appropriate import for ContextAwareIndexPlugin), change
org.opensearch.Version.CURRENT to the already-used Version.CURRENT for
consistency, and update the plugin version string "1.8" to match the other
plugins (e.g., "21") so PluginInfo, ContextAwareIndexPlugin, and Version.CURRENT
usages compile and are consistent.
| final List<Directory> directoryToCombine = new ArrayList<>(); | ||
| final AtomicLong pendingNumDocsByOldChildWriter = new AtomicLong(); | ||
| for (CompositeIndexWriter.DisposableIndexWriter childDisposableWriter : markForRefreshIndexWritersMap.values()) { | ||
| directoryToCombine = childDisposableWriter.getIndexWriter().getDirectory(); | ||
| directoryToCombine.add(childDisposableWriter.getIndexWriter().getDirectory()); | ||
| childDisposableWriter.getIndexWriter().close(); | ||
| long pendingNumDocsByOldChildWriter = childDisposableWriter.getIndexWriter().getPendingNumDocs(); | ||
| accumulatingIndexWriter.addIndexes(directoryToCombine); | ||
| pendingNumDocsByOldChildWriter.addAndGet(childDisposableWriter.getIndexWriter().getPendingNumDocs()); | ||
| } |
There was a problem hiding this comment.
Read pending docs before closing child writers.
Line 552 closes the IndexWriter before Line 553 calls getPendingNumDocs(). That can throw AlreadyClosedException or return an invalid value, leading to incorrect pending-doc accounting. Fetch pending count before closing.
✅ Proposed fix
- for (CompositeIndexWriter.DisposableIndexWriter childDisposableWriter : markForRefreshIndexWritersMap.values()) {
- directoryToCombine.add(childDisposableWriter.getIndexWriter().getDirectory());
- childDisposableWriter.getIndexWriter().close();
- pendingNumDocsByOldChildWriter.addAndGet(childDisposableWriter.getIndexWriter().getPendingNumDocs());
- }
+ for (CompositeIndexWriter.DisposableIndexWriter childDisposableWriter : markForRefreshIndexWritersMap.values()) {
+ IndexWriter childWriter = childDisposableWriter.getIndexWriter();
+ directoryToCombine.add(childWriter.getDirectory());
+ pendingNumDocsByOldChildWriter.addAndGet(childWriter.getPendingNumDocs());
+ childWriter.close();
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| final List<Directory> directoryToCombine = new ArrayList<>(); | |
| final AtomicLong pendingNumDocsByOldChildWriter = new AtomicLong(); | |
| for (CompositeIndexWriter.DisposableIndexWriter childDisposableWriter : markForRefreshIndexWritersMap.values()) { | |
| directoryToCombine = childDisposableWriter.getIndexWriter().getDirectory(); | |
| directoryToCombine.add(childDisposableWriter.getIndexWriter().getDirectory()); | |
| childDisposableWriter.getIndexWriter().close(); | |
| long pendingNumDocsByOldChildWriter = childDisposableWriter.getIndexWriter().getPendingNumDocs(); | |
| accumulatingIndexWriter.addIndexes(directoryToCombine); | |
| pendingNumDocsByOldChildWriter.addAndGet(childDisposableWriter.getIndexWriter().getPendingNumDocs()); | |
| } | |
| final List<Directory> directoryToCombine = new ArrayList<>(); | |
| final AtomicLong pendingNumDocsByOldChildWriter = new AtomicLong(); | |
| for (CompositeIndexWriter.DisposableIndexWriter childDisposableWriter : markForRefreshIndexWritersMap.values()) { | |
| IndexWriter childWriter = childDisposableWriter.getIndexWriter(); | |
| directoryToCombine.add(childWriter.getDirectory()); | |
| pendingNumDocsByOldChildWriter.addAndGet(childWriter.getPendingNumDocs()); | |
| childWriter.close(); | |
| } |
🤖 Prompt for AI Agents
In `@server/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.java`
around lines 548 - 554, The loop in CompositeIndexWriter is closing each child
IndexWriter before calling getPendingNumDocs(), which can throw
AlreadyClosedException; change the loop to first obtain a local reference to the
child writer (e.g., IndexWriter iw = childDisposableWriter.getIndexWriter()),
then call iw.getPendingNumDocs() and add to pendingNumDocsByOldChildWriter, then
collect iw.getDirectory() and finally call iw.close(); update uses of
getIndexWriter() in the loop to that local variable (refer to
markForRefreshIndexWritersMap, DisposableIndexWriter, getIndexWriter(),
getPendingNumDocs(), getDirectory(), close()).
| protected Collection<Class<? extends Plugin>> nodePlugins() { | ||
| return Arrays.asList(MockRepository.Plugin.class); | ||
| return List.of(MockRepository.Plugin.class, ContextAwareIndexPlugin.class); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check imports in AbstractSnapshotIntegTestCase.java
head -100 test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java | grep -n "import"Repository: opensearch-project/OpenSearch
Length of output: 3599
🏁 Script executed:
#!/bin/bash
# Search for ContextAwareIndexPlugin references
rg -n "ContextAwareIndexPlugin" test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.javaRepository: opensearch-project/OpenSearch
Length of output: 160
🏁 Script executed:
#!/bin/bash
# Find where ContextAwareIndexPlugin is defined
rg -n "class ContextAwareIndexPlugin" --type javaRepository: opensearch-project/OpenSearch
Length of output: 220
🏁 Script executed:
#!/bin/bash
# Check the class declaration and what AbstractSnapshotIntegTestCase extends
rg -n "^public.*class AbstractSnapshotIntegTestCase" -A 2 test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.javaRepository: opensearch-project/OpenSearch
Length of output: 272
🏁 Script executed:
#!/bin/bash
# Check if ContextAwareIndexPlugin needs to be imported or if it's statically available
# Look for any static import or reference pattern
grep -n "static.*import\|OpenSearchIntegTestCase" test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java | head -20Repository: opensearch-project/OpenSearch
Length of output: 271
🏁 Script executed:
#!/bin/bash
# Check what ParameterizedStaticSettingsOpenSearchIntegTestCase extends
rg -n "^public.*class ParameterizedStaticSettingsOpenSearchIntegTestCase" -A 1 test/framework/src/main/java/org/opensearch/test/ParameterizedStaticSettingsOpenSearchIntegTestCase.javaRepository: opensearch-project/OpenSearch
Length of output: 199
🏁 Script executed:
#!/bin/bash
# Also check OpenSearchIntegTestCase hierarchy
rg -n "^public.*class OpenSearchIntegTestCase" -A 1 test/framework/src/main/java/org/opensearch/test/OpenSearchIntegTestCase.java | head -5Repository: opensearch-project/OpenSearch
Length of output: 156
🏁 Script executed:
#!/bin/bash
# Check ParameterizedOpenSearchIntegTestCase extends clause
rg -n "^public.*class ParameterizedOpenSearchIntegTestCase" -A 1 test/framework/src/main/java/org/opensearch/test/ParameterizedOpenSearchIntegTestCase.javaRepository: opensearch-project/OpenSearch
Length of output: 55
🏁 Script executed:
#!/bin/bash
# Check if ContextAwareIndexPlugin can be accessed without import by testing Java's nested class accessibility rules
# Let's verify if the code would compile by checking if there's a pattern of using OpenSearchIntegTestCase.ContextAwareIndexPlugin elsewhere
rg -n "OpenSearchIntegTestCase\.ContextAwareIndexPlugin" --type javaRepository: opensearch-project/OpenSearch
Length of output: 419
Fix the compilation error: ContextAwareIndexPlugin cannot be resolved.
Line 158 uses ContextAwareIndexPlugin.class without importing it. Since ContextAwareIndexPlugin is a public static inner class of OpenSearchIntegTestCase (not inherited by AbstractSnapshotIntegTestCase), it must be either imported explicitly or referenced with its full qualifier. Other usages in the codebase (e.g., OpenSearchSingleNodeTestCase.java, AbstractMultiClustersTestCase.java) use the fully qualified form: OpenSearchIntegTestCase.ContextAwareIndexPlugin.class. Apply the same pattern here.
🤖 Prompt for AI Agents
In
`@test/framework/src/main/java/org/opensearch/snapshots/AbstractSnapshotIntegTestCase.java`
around lines 157 - 159, Replace the unresolved ContextAwareIndexPlugin reference
in nodePlugins() by using the fully qualified nested class used elsewhere:
change ContextAwareIndexPlugin.class to
OpenSearchIntegTestCase.ContextAwareIndexPlugin.class (so nodePlugins() returns
List.of(MockRepository.Plugin.class,
OpenSearchIntegTestCase.ContextAwareIndexPlugin.class)); this avoids adding an
import and matches other usages.
|
❌ Gradle check result for 290d970: 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-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| .getSetting(INDEX_NAME, IndexSettings.INDEX_CONTEXT_AWARE_ENABLED_SETTING.getKey()); | ||
| if (isCASEnabled != null && isCASEnabled.equals("true")) { | ||
| // For CAS enabled domains, there will be an extra refresh for each OpenSearch flush call. | ||
| extraMetadataFilesDueToFlush += indexingStats.get(FLUSHED_OPERATIONS); |
Check failure
Code scanning / CodeQL
Implicit narrowing conversion in compound assignment
| .getSetting(INDEX_NAME, IndexSettings.INDEX_CONTEXT_AWARE_ENABLED_SETTING.getKey()); | ||
| if (isCASEnabled != null && isCASEnabled.equals("true")) { | ||
| // For CAS enabled domains, there will be an extra refresh for each OpenSearch flush call. | ||
| extraMetadataFilesDueToFlush += indexingStats.get(FLUSHED_OPERATIONS); |
Check failure
Code scanning / CodeQL
Implicit narrowing conversion in compound assignment
| .getSetting(INDEX_NAME, IndexSettings.INDEX_CONTEXT_AWARE_ENABLED_SETTING.getKey()); | ||
| if (isCASEnabled != null && isCASEnabled.equals("true")) { | ||
| // For CAS enabled domains, there will be an extra refresh for each OpenSearch flush call. | ||
| extraMetadataFilesDueToFlush += indexingStats.get(FLUSHED_OPERATIONS); |
Check failure
Code scanning / CodeQL
Implicit narrowing conversion in compound assignment
RS146BIJAY
force-pushed
the
enable-test-case-final
branch
from
290d970 to
2077eea
Compare
|
❌ Gradle check result for 2077eea: 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? |
Signed-off-by: RS146BIJAY <rishavsagar4b1@gmail.com>
RS146BIJAY
force-pushed
the
enable-test-case-final
branch
from
2077eea to
8ec0419
Compare
|
❌ Gradle check result for 8ec0419: 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? |
1 participant
Description
This PR fixes following: