[Workload Management][Rule Based Autotagging] Scroll API support in autotagging#20151
[Workload Management][Rule Based Autotagging] Scroll API support in autotagging#20151laminelam wants to merge 5 commits intoopensearch-project:mainfrom
Conversation
Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
github-actions
bot
added
enhancement
|
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
📝 WalkthroughWalkthroughAdds propagation of the original indices through scroll IDs, exposes original indices via a new TransportOriginalIndicesAction and ActionRequestMetadata.originalIndices(), updates search/scroll parsing to carry original indices, and extends workload-management tagging to handle SearchScrollRequest (tests added; one test duplicated). Changes
Sequence DiagramsequenceDiagram
participant Client
participant Transport as TransportSearchScrollAction
participant SSR as SearchScrollRequest
participant PSID as ParsedScrollId
participant Metadata as ActionRequestMetadata
participant ATAF as AutoTaggingActionFilter
participant WLS as RuleProcessingService
Client->>Transport: send SearchRequest -> initial search (build scrollId with indices)
Transport-->>Client: response with scrollId
Client->>Transport: SearchScrollRequest (scrollId)
Transport->>SSR: request.parseScrollId()
SSR->>PSID: parse/cached parse
PSID-->>SSR: ParsedScrollId (includes originalIndices)
SSR-->>Transport: parsed result
Transport->>Metadata: attach originalIndices to request metadata
Transport->>ATAF: pass request through filter
ATAF->>Metadata: read originalIndices
ATAF->>WLS: evaluateLabel(using INDEX_PATTERN attribute)
WLS-->>ATAF: workload group label
ATAF-->>Transport: set workload header / proceed
Transport-->>Client: scroll response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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: 1
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/action/search/SearchScrollRequest.java (1)
101-111: Bug: Cache is not invalidated when scrollId is changed.If
scrollId(String)is called afterparseScrollId()has already been invoked, the cachedparsedScrollIdwill become stale and return results for the old scroll ID.Apply this diff to clear the cache when scrollId changes:
public SearchScrollRequest scrollId(String scrollId) { this.scrollId = scrollId; + this.parsedScrollId = null; return this; }
🧹 Nitpick comments (5)
plugins/workload-management/src/internalClusterTest/java/org/opensearch/plugin/wlm/WlmAutoTaggingIT.java (1)
442-477: Scroll auto‑tagging IT is correct; consider clearing scroll context as a small improvementThis test is well-structured and mirrors the existing WLM auto-tagging tests: it enables WLM, creates a workload group and rule, indexes data, and then verifies that both the initial search (with
setScroll(...)) and the subsequentprepareSearchScroll(...)call increase completions. The assertions onscrollIdand monotonic increases ingetCompletions(...)make sense and exercise the new scroll-tagging path end-to-end.One minor improvement you might consider:
- After finishing with the scroll, clear the scroll context (e.g.,
client().prepareClearScroll().addScrollId(scrollId).get();), possibly in atry/finally, to avoid leaving scrolls open across potentialassertBusyretries. It’s not a blocker for a small IT but would make the test more self-contained and resource-friendly.server/src/test/java/org/opensearch/action/search/SearchScrollAsyncActionTests.java (1)
481-485: Constructor update is correct, but consider adding test coverage for non-empty originalIndices.The update to match the new
ParsedScrollIdconstructor signature is correct. However, this test file doesn't exercise the neworiginalIndicesfunctionality. Consider adding a test case that verifies scroll behavior whenoriginalIndicesis populated, to ensure the autotagging flow works end-to-end.server/src/main/java/org/opensearch/action/search/ParsedScrollId.java (1)
56-79: Consider defensive copies for array field in public API.Since
ParsedScrollIdis annotated with@PublicApi, external callers could mutate theoriginalIndicesarray after construction or after callinggetOriginalIndices(). Consider defensive copying for better encapsulation.- ParsedScrollId(String source, String type, SearchContextIdForNode[] context, String[] originalIndices) { + ParsedScrollId(String source, String type, SearchContextIdForNode[] context, String[] originalIndices) { this.source = source; this.type = type; this.context = context; - this.originalIndices = originalIndices; + this.originalIndices = originalIndices != null ? originalIndices.clone() : null; }public String[] getOriginalIndices() { - return originalIndices; + return originalIndices != null ? originalIndices.clone() : null; }server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java (1)
137-146: Verify the deserialization logic aligns with the position check.The conditional reading of
originalIndicesis correct, but ensure the position check on line 148 (in.getPosition() != bytes.length) still functions as intended:
- When
originalIndicesare absent, position should equalbytes.lengthbefore line 148.- When
originalIndicesare present, they must be fully consumed so position equalsbytes.length.Consider adding a unit test that verifies scroll IDs with and without
originalIndicespass the position check.plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/AutoTaggingActionFilter.java (1)
100-119: Consider extracting the anonymous AttributeExtractor to improve readability.The inline anonymous
AttributeExtractorimplementation is functionally correct but could be refactored into a named helper method (e.g.,createResolvedIndicesExtractor) to improve code clarity and testability.Example refactor:
private static AttributeExtractor<String> createResolvedIndicesExtractor(Set<String> indexNames) { return new AttributeExtractor<>() { @Override public Attribute getAttribute() { return RuleAttribute.INDEX_PATTERN; } @Override public Iterable<String> extract() { return indexNames; } @Override public LogicalOperator getLogicalOperator() { return LogicalOperator.AND; } }; }Then replace lines 104-119 with:
- attributeExtractors.add(new AttributeExtractor<>() { - @Override - public Attribute getAttribute() { - return RuleAttribute.INDEX_PATTERN; - } - - @Override - public Iterable<String> extract() { - return names; - } - - @Override - public LogicalOperator getLogicalOperator() { - return LogicalOperator.AND; - } - }); + attributeExtractors.add(createResolvedIndicesExtractor(names));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
plugins/workload-management/src/internalClusterTest/java/org/opensearch/plugin/wlm/WlmAutoTaggingIT.java(1 hunks)plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/AutoTaggingActionFilter.java(3 hunks)plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/AutoTaggingActionFilterTests.java(2 hunks)server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java(1 hunks)server/src/main/java/org/opensearch/action/search/ParsedScrollId.java(2 hunks)server/src/main/java/org/opensearch/action/search/SearchScrollRequest.java(2 hunks)server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java(3 hunks)server/src/main/java/org/opensearch/action/search/TransportSearchScrollAction.java(4 hunks)server/src/test/java/org/opensearch/action/search/ParsedScrollIdTests.java(1 hunks)server/src/test/java/org/opensearch/action/search/SearchScrollAsyncActionTests.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java (1)
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java (1)
TransportSearchHelper(55-160)
plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/AutoTaggingActionFilterTests.java (1)
server/src/main/java/org/opensearch/action/support/ActionRequestMetadata.java (1)
ActionRequestMetadata(21-61)
server/src/main/java/org/opensearch/action/search/SearchScrollRequest.java (1)
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java (1)
TransportSearchHelper(55-160)
⏰ 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: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: detect-breaking-change
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: Analyze (java)
🔇 Additional comments (11)
server/src/main/java/org/opensearch/action/search/AbstractSearchAsyncAction.java (1)
777-779: LGTM! Indices are now correctly propagated to scroll ID construction.The change correctly passes
request.indices()tobuildScrollId, enabling the scroll ID to carry original index information for autotagging purposes. TheTransportSearchHelper.buildScrollIdmethod properly handles null/empty arrays with version gating.server/src/test/java/org/opensearch/action/search/ParsedScrollIdTests.java (1)
53-58: LGTM! Constructor call updated correctly.The test correctly uses the new constructor signature. The
hasLocalIndices()test remains valid since it doesn't depend onoriginalIndices. Consider adding a separate test forgetOriginalIndices()to verify it returns the expected values.server/src/main/java/org/opensearch/action/search/TransportSearchScrollAction.java (3)
54-56: LGTM! TransportIndicesResolvingAction implementation enables autotagging for scroll requests.The class now correctly implements
TransportIndicesResolvingAction<SearchScrollRequest>, enabling the workload management system to resolve indices for scroll requests and apply appropriate tags.
87-87: Good change to use cached parsing.Using
request.parseScrollId()instead ofTransportSearchHelper.parseScrollId(request.scrollId())leverages the lazy caching mechanism, avoiding redundant parsing ifresolveIndices()was called earlier in the request lifecycle.
123-143: Robust implementation with proper defensive handling.The
resolveIndices()implementation correctly:
- Guards against null/empty scroll ID strings
- Handles null parsed result
- Returns
OptionallyResolvedIndices.unknown()for missing originalIndices- Catches all exceptions to prevent failures from blocking request processing
One minor note: the null check on line 132 (
parsed == null) should never be true given the currentparseScrollId()implementation throws on parse failure rather than returning null. However, keeping it is good defensive programming.server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java (4)
65-67: LGTM!Delegating to the new overload with
nullfororiginalIndicesmaintains backward compatibility for existing callers.
151-151: Verify the ParsedScrollId constructor signature.The code at line 151 calls
ParsedScrollId(scrollId, type, context, originalIndices)with four parameters. Ensure that theParsedScrollIdclass constructor has been updated to accept all four parameters, includingString[] originalIndicesas the fourth parameter.
63-63: Verify that Version.V_3_3_2 exists and is the intended version.This constant gates a wire format change for including originalIndices in scroll IDs. Ensure that
Version.V_3_3_2is defined in theVersionclass and that this is the correct minimum version for the feature.
88-99: No action required — scroll IDs are designed as opaque, versioned tokens.The code correctly uses version gating to conditionally serialize
originalIndices. OpenSearch's documented design explicitly treats scroll IDs as opaque, internally-versioned structures that are not intended to be cross-version compatible. Scroll IDs should never be parsed or shared across versions; they are opaque tokens whose internal format can change arbitrarily. Each version reads exactly what it wrote, and the conditional serialization ensures older nodes do not encounter unexpected fields.plugins/workload-management/src/main/java/org/opensearch/plugin/wlm/AutoTaggingActionFilter.java (1)
88-90: LGTM: Request type validation extended correctly.The validation logic now correctly includes both
SearchRequestandSearchScrollRequest, allowing scroll operations to be tagged by the workload management system.plugins/workload-management/src/test/java/org/opensearch/plugin/wlm/AutoTaggingActionFilterTests.java (1)
98-115: Additional test coverage should verify fallback behavior and validate extracted indices.The test correctly verifies the happy path for
SearchScrollRequestwith resolved indices and workload tagging. However, without access to the codebase for verification, I cannot confirm:
- Whether a fallback scenario test is actually needed (requires reviewing lines 122-125 in
AutoTaggingActionFilter.java)- Whether the API references in the suggested example (
OptionallyResolvedIndices.unknown()) are valid- Whether the
ResolvedIndices.of()factory method exists as usedThe suggestion to verify actual indices passed to
evaluateLabelvia argument captors remains valid and should be considered if the fallback scenario is indeed a code path that needs testing.
...ins/workload-management/src/main/java/org/opensearch/plugin/wlm/AutoTaggingActionFilter.java
Outdated
Show resolved
Hide resolved
|
❌ Gradle check result for 2cc9bb7: 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? |
|
@kaushalmahi12 |
|
This PR is stalled because it has been open for 30 days with no activity. |
|
Hi @andrross |
|
@kaushalmahi12 Can you take a look? |
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Outdated
Show resolved
Hide resolved
|
I will look into it this week. @dzane17 Can you do the first round of review on this ? |
Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
|
❌ Gradle check result for 7497b6a: 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? |
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
|
❌ Gradle check result for 7c00443: 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? |
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Show resolved
Hide resolved
|
❌ Gradle check result for 7c00443: 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? |
...ins/workload-management/src/main/java/org/opensearch/plugin/wlm/AutoTaggingActionFilter.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/search/TransportSearchHelper.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/action/search/TransportSearchScrollAction.java
Outdated
Show resolved
Hide resolved
remove TransportOriginalIndicesAction Signed-off-by: Lamine Idjeraoui <lidjeraoui@apple.com>
|
❌ Gradle check result for ce35d03: 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? |
jainankitk
left a comment
jainankitk
left a comment
There was a problem hiding this comment.
Checks failed due to spotless failure:
* What went wrong:
Execution failed for task ':plugins:workload-management:spotlessJavaCheck'.
> The following files had format violations:
src/internalClusterTest/java/org/opensearch/plugin/wlm/WlmAutoTaggingIT.java
@@ -467,24 +467,16 @@
············try·{
················int·afterInitialSearch·=·getCompletions(workloadGroupId);
-················assertTrue(
-····················"Expected·completions·to·increase·after·initial·search·with·scroll",
-····················afterInitialSearch·>·completionsBefore
-················);
Please Run './gradlew spotlessApply' to fix all violations. Also, can you add entry to the changelog?
|
❌ Gradle check result for 71f2255: 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? |
|
Thanks @laminelam for making this change just fix the the gradle checks, rest of it looks good to me. |
Thanks @kaushalmahi12 |
andrross
left a comment
andrross
left a comment
There was a problem hiding this comment.
Looks good, just need to rebase, fix the version guard, and add the changelog entry.
|
|
||
| private static final String INCLUDE_CONTEXT_UUID = "include_context_uuid"; | ||
|
|
||
| public static final Version INDICES_IN_SCROLL_ID_VERSION = Version.V_3_4_0; |
5 participants
Resolves #8362
Description
[Describe what this change achieves]
Check List
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.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.