Prevent criteria update for context aware indices - #20250
Conversation
|
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 validation to prevent updating grouping criteria in context-aware indices. It extends Changes
Sequence Diagram(s)sequenceDiagram
participant InternalEngine
participant VersionsAndSeqNoResolver
participant CriteriaBasedCodec
participant CompositeIndexWriter
InternalEngine->>InternalEngine: resolveDocVersion (Index op with grouping criteria)
InternalEngine->>InternalEngine: Extract currentCriteria from op.docs()
InternalEngine->>VersionsAndSeqNoResolver: loadDocIdAndVersion(reader, term, ..., currentCriteria)
rect rgb(200, 220, 240)
Note over VersionsAndSeqNoResolver: New Validation Flow
VersionsAndSeqNoResolver->>VersionsAndSeqNoResolver: Find doc version
alt Non-zero version & criteria provided
VersionsAndSeqNoResolver->>CriteriaBasedCodec: Unwrap reader & read previous criteria<br/>from BUCKET_NAME
CriteriaBasedCodec-->>VersionsAndSeqNoResolver: previousCriteria
alt previousCriteria != currentCriteria
VersionsAndSeqNoResolver->>VersionsAndSeqNoResolver: throw UnsupportedOperationException
else Match
VersionsAndSeqNoResolver-->>InternalEngine: DocIdAndVersion
end
else No criteria or tombstone
VersionsAndSeqNoResolver-->>InternalEngine: DocIdAndVersion
end
end
InternalEngine->>InternalEngine: Post-load validation for Index ops
alt Non-tombstone version (> 0)
InternalEngine->>CompositeIndexWriter: getCriteriaUnderLock(uid)
CompositeIndexWriter-->>InternalEngine: previousCriteria (or null)
alt previousCriteria exists & differs
InternalEngine->>InternalEngine: throw UnsupportedOperationException
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
|
❌ Gradle check result for fbfcac0: 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? |
961319b to
5a273d7
Compare
|
❌ Gradle check result for 5a273d7: 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? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
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/engine/CompositeIndexWriter.java (1)
625-645: Fix criteria lookup to use the correct map instance instead of always reading from currentIn
getIndexWriterForIdFromLookup, you hold the read lock forindexWriterLookupbut then callgetCurrentCriteria(uid), which always reads fromliveIndexWriterDeletesMap.current. WhenindexWriterLookupisliveIndexWriterDeletesMap.old(viagetIndexWriterForIdFromOld), this creates two problems:
Lock doesn't protect the data being read: You lock the old map but read from the current map, leaving the criteria lookup unprotected.
Data loss for previously-indexed documents: For a document whose last write occurred in a previous refresh cycle, its criteria exists only in
old.criteria. SincegetCurrentCriteriaonly checkscurrent.criteria, it returns null for both old and current lookups, causing the delete to skip the child writer containing the prior version. This violates the class javadoc intent to "perform a partial soft delete...on the IndexWriters containing the previous version of the document."Replace
getCurrentCriteria(uid)withindexWriterLookup.getCriteriaForDoc(uid)to read from the map you're actually holding the lock for, or use the helper method that respects which map you're reading from:String criteria = getCriteriaUnderLock(uid); // resolves from current then old DisposableIndexWriter disposableIndexWriter = indexWriterLookup.getIndexWriterForCriteria(criteria);This also applies to lines 651–668.
🧹 Nitpick comments (7)
server/src/main/java/org/opensearch/common/lucene/uid/VersionsAndSeqNoResolver.java (1)
35-46: Criteria validation in loadDocIdAndVersion matches intent; consider safer reader handling and null‑attribute guardThe new
currentCriteriagate and bucket comparison accurately enforce “no grouping-criteria updates” for live docs when a criteria is provided, while leaving existing behavior unchanged whencurrentCriteria == null.Two refinements worth considering:
Avoid hard-casting the unwrapped reader
SegmentReader unwrappedReader = (SegmentReader) (FilterLeafReader.unwrap(leaf.reader()));This assumes the unwrapped reader is always a
SegmentReader. To decouple from Lucene’s concrete leaf type, it would be safer and more idiomatic to use the existing helper you already rely on elsewhere (Lucene.segmentReader(...)) or a similar utility that performs the appropriate checks and unwrapping internally, instead of a direct cast.Handle missing BUCKET_NAME attributes defensively
String prevCriteria = unwrappedReader.getSegmentInfo().info.getAttribute(CriteriaBasedCodec.BUCKET_NAME); assert prevCriteria != null; if (prevCriteria.equals(currentCriteria) == false) { ... }The
assertwon’t fire in production; if for any reasonBUCKET_NAMEis absent (e.g., older segments, misconfigured codec, or future changes), this becomes an NPE at the.equalscall in a hot update path. If such segments are even theoretically possible, consider either:
- Treating
prevCriteria == nullas “no criteria recorded yet” (skip the check), or- Throwing a more explicit
ImmutableCriteriaExceptionwhenprevCriteriais null, rather than an unchecked NPE.These changes would make the criteria enforcement more robust to unexpected segment states without changing the intended semantics.
Also applies to: 162-189
server/src/main/java/org/opensearch/index/engine/InternalEngine.java (1)
784-796: resolveDocVersion’s criteria checks are well‑placed; consider tightening assumptions and gatingThe two new checks (one when falling back to Lucene, one when hitting the version map) are consistent with the goal: prevent changing grouping criteria while still allowing normal updates where the criteria is unchanged, and allowing recreate‑after‑delete (via
previousCriteria == nullcontract).A few focused suggestions:
Explicitly gate on context‑aware indices (optional but clearer/safer)
Today, both branches run wheneverop instanceof Index, relying on:
index.docs().get(0).getGroupingCriteria()returningnullfor non–context‑aware indices, anddocumentIndexWriter.getCriteriaUnderLock(...)returningnullin those cases.
To decouple this method from those assumptions and avoid unnecessary work on non–context‑aware indices, consider wrapping both criteria blocks withif (isContextAwareEnabled) { ... }.Document the “first doc” assumption
Both branches take criteria fromindex.docs().get(0). If the invariant is that all docs in theParsedDocumentshare the same grouping criteria (which is likely for nested docs), it would help future maintainers if this is stated in a brief comment near the first usage.(Minor) Avoid recomputing currentCriteria twice
You currently recomputecurrentCriteriain both theversionValue == nullandelse if (op instanceof Index && versionValue.version > 0)paths. You could factor it once at the top of the method forIndexoperations to keep things DRY, though this is purely cosmetic.Functionally the changes look correct and align with the PR’s behavior goals; these are about making the contract and scope more explicit and maintainable.
Also applies to: 805-817
server/src/main/java/org/opensearch/index/engine/ImmutableCriteriaException.java (1)
1-41: ImmutableCriteriaException definition is appropriate for the hot update pathThe class shape (OpenSearchException subclass, streaming constructor,
fillInStackTraceoverride, andBAD_REQUESTstatus) is consistent with other engine exceptions and well‑suited for a frequently‑thrown validation error.One minor polish you might consider: the comment in
fillInStackTracestill references “VCEEs”; updating the wording to refer to immutable‑criteria exceptions would avoid confusion, but it’s non‑functional.server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (1)
8680-8731: New grouping-criteria immutability tests cover both forbidden and allowed cases; consider tightening assertionsThe two tests cleanly validate:
- An in-place change of grouping criteria for the same id is rejected with
ImmutableCriteriaException.- After a delete tombstone and refresh, re‑indexing the same id with a different grouping criteria is accepted.
To make these tests more robust and self‑documenting, consider:
- In
testDoesNotAllowGroupingCriteriaUpdate, optionally asserting on the exception message (or a distinguishing property) to guard against unrelated failures being surfaced as the same type.- In
testAllowGroupingCriteriaUpdateWithTombstone, adding a follow‑upgetor search assertion that the final document for id"2"is present, so the test fails if a future change silently drops the write even though no exception is thrown.These are test-strengthening tweaks; current behavior is reasonable as-is.
CHANGELOG.md (1)
103-103: Minor wording tweak: hyphenate “context-aware”For consistency with other entries and standard English, consider:
- Prevent criteria update for context-aware indices ([#20250](...))server/src/main/java/org/opensearch/index/engine/DocumentIndexWriter.java (1)
16-17: New getCriteriaUnderLock API is appropriate; consider documenting semanticsAdding
String getCriteriaUnderLock(BytesRef uid)to the interface cleanly exposes grouping criteria to callers (e.g., InternalEngine) and is implemented by both LuceneIndexWriter and CompositeIndexWriter.To make usage clearer, consider adding brief Javadoc explaining:
- Whether
nullis a valid/expected return (e.g., for non–context-aware writers).- Any expectations about locks (e.g., must the caller already hold a keyed lock, or is it a best-effort snapshot).
Also applies to: 95-97
server/src/test/java/org/opensearch/common/lucene/uid/VersionsTests.java (1)
80-81: Tests correctly adapt to the new loadDocIdAndVersion signatureUpdating all
loadDocIdAndVersioncalls to passnullfor the newcurrentCriteriaparameter keeps existing test behaviour intact while matching the new API shape.If not already covered in engine-level tests, consider adding a focused unit test in this suite that passes a non-null
currentCriteriaand asserts the expectedImmutableCriteriaExceptionwhen criteria mismatch, to exercise that branch ofVersionsAndSeqNoResolver.Also applies to: 89-90, 100-101, 112-113, 116-117, 144-145, 151-152, 155-156, 175-176, 178-179, 201-202, 205-206
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
CHANGELOG.md(1 hunks)server/src/main/java/org/opensearch/OpenSearchServerException.java(2 hunks)server/src/main/java/org/opensearch/common/lucene/uid/VersionsAndSeqNoResolver.java(3 hunks)server/src/main/java/org/opensearch/index/engine/CompositeIndexWriter.java(2 hunks)server/src/main/java/org/opensearch/index/engine/DocumentIndexWriter.java(2 hunks)server/src/main/java/org/opensearch/index/engine/Engine.java(2 hunks)server/src/main/java/org/opensearch/index/engine/ImmutableCriteriaException.java(1 hunks)server/src/main/java/org/opensearch/index/engine/InternalEngine.java(2 hunks)server/src/main/java/org/opensearch/index/engine/LuceneIndexWriter.java(2 hunks)server/src/test/java/org/opensearch/ExceptionSerializationTests.java(2 hunks)server/src/test/java/org/opensearch/common/lucene/uid/VersionsTests.java(7 hunks)server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java(4 hunks)test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
server/src/test/java/org/opensearch/ExceptionSerializationTests.java (1)
server/src/main/java/org/opensearch/index/engine/ImmutableCriteriaException.java (1)
ImmutableCriteriaException(22-41)
server/src/main/java/org/opensearch/index/engine/Engine.java (1)
server/src/main/java/org/opensearch/common/lucene/uid/VersionsAndSeqNoResolver.java (1)
VersionsAndSeqNoResolver(57-210)
server/src/main/java/org/opensearch/common/lucene/uid/VersionsAndSeqNoResolver.java (2)
server/src/main/java/org/opensearch/index/codec/CriteriaBasedCodec.java (1)
CriteriaBasedCodec(26-72)server/src/main/java/org/opensearch/index/engine/ImmutableCriteriaException.java (1)
ImmutableCriteriaException(22-41)
server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (1)
server/src/main/java/org/opensearch/common/lucene/uid/VersionsAndSeqNoResolver.java (1)
VersionsAndSeqNoResolver(57-210)
server/src/main/java/org/opensearch/index/engine/InternalEngine.java (1)
server/src/main/java/org/opensearch/common/lucene/uid/VersionsAndSeqNoResolver.java (1)
VersionsAndSeqNoResolver(57-210)
server/src/main/java/org/opensearch/OpenSearchServerException.java (1)
server/src/main/java/org/opensearch/index/engine/ImmutableCriteriaException.java (1)
ImmutableCriteriaException(22-41)
server/src/test/java/org/opensearch/common/lucene/uid/VersionsTests.java (1)
server/src/main/java/org/opensearch/common/lucene/uid/VersionsAndSeqNoResolver.java (1)
VersionsAndSeqNoResolver(57-210)
🪛 LanguageTool
CHANGELOG.md
[uncategorized] ~103-~103: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...l/20140)) - Prevent criteria update for context aware indices ([#20250](https://github.com/op...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
⏰ 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). (1)
- GitHub Check: gradle-check
🔇 Additional comments (8)
server/src/main/java/org/opensearch/index/engine/Engine.java (1)
335-340: loadDocIdAndVersion wiring with new parameter looks correctBoth usages (
getMaxSeqNoFromSearcherandgetFromSearcher) now passtrue, null, preserving prior behavior while adapting to the extended signature and intentionally skipping criteria validation on these read-only/internal paths. No issues spotted.Also applies to: 708-708
server/src/test/java/org/opensearch/index/engine/InternalEngineTests.java (3)
1879-1879: UpdatedloadDocIdAndVersioncall correctly passescurrentCriteriaas nullUsing the new 4‑arg overload with
nullforcurrentCriteriakeeps this test focused on the pruned‑IDs behavior and ensures the new criteria check is not triggered here. No changes needed.
8595-8597: Switch totestContextSpecificDocumentaligns composite-writer failure test with context-aware indicesCreating all three documents via
testContextSpecificDocument("grouping_criteria")is consistent with exercising the composite writer path under context‑aware settings, without altering the test’s core “addIndexes throws → shard fails” behavior. Looks good.
8641-8643: Context-specific documents are appropriate for the update-path composite-writer failure testUsing
testContextSpecificDocument("grouping_criteria")for the append‑then‑update scenario maintains the original intent (failure onaddIndexesduring update) while ensuring the setup matches the context‑aware index configuration. No issues spotted.test/framework/src/main/java/org/opensearch/index/engine/EngineTestCase.java (1)
369-373: Parametrizing testContextSpecificDocument is fineAllowing callers to pass
groupingCriteriainstead of hard-coding it improves test flexibility without changing semantics; no issues spotted.server/src/test/java/org/opensearch/ExceptionSerializationTests.java (1)
90-90: ImmutableCriteriaException serialization mapping looks correctThe new import and
ids.put(10003, ImmutableCriteriaException.class);entry align with the server-side registration (base 10000 + 3); this keeps the bidirectional ID ↔ class mapping consistent.Also applies to: 730-910
server/src/main/java/org/opensearch/OpenSearchServerException.java (1)
28-29: ImmutableCriteriaException registration is consistentRegistering
ImmutableCriteriaExceptionwith idCUSTOM_ELASTICSEARCH_EXCEPTIONS_BASE_ID + 3(10003) and versionV_3_4_0matches the test-side mapping and keeps IDs in the custom range without conflicts.Also applies to: 1255-1262
server/src/main/java/org/opensearch/index/engine/LuceneIndexWriter.java (1)
15-16: Null-returning getCriteriaUnderLock is appropriate for LuceneIndexWriterImplementing
getCriteriaUnderLock(BytesRef uid)to always returnnullcorrectly reflects that plain LuceneIndexWriter doesn’t track grouping criteria; this ensures criteria immutability checks are only applied when a context-aware writer provides a non-null value.Also applies to: 230-232
|
❌ Gradle check result for ff990c9: 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? |
6772f3d to
47c7a3f
Compare
|
❌ Gradle check result for 47c7a3f: 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? |
47c7a3f to
9b69b1f
Compare
|
❌ Gradle check result for 9b69b1f: 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? |
9b69b1f to
510bd9c
Compare
|
❌ Gradle check result for 510bd9c: 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? |
510bd9c to
885d8a8
Compare
|
❌ Gradle check result for 885d8a8: 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>
885d8a8 to
1bc19a3
Compare
…oject#20250) Signed-off-by: RS146BIJAY <rishavsagar4b1@gmail.com>
…oject#20250) Signed-off-by: RS146BIJAY <rishavsagar4b1@gmail.com>
…oject#20250) Signed-off-by: RS146BIJAY <rishavsagar4b1@gmail.com>
3 participants
Description
This modification prevents updates to fields that determine a document's grouping criteria. By blocking such updates, we eliminate the need for complex version synchronization across multiple IndexWriters. This approach simplifies version management, since the latest version of a document will always reside in the group-specific writer marked for refresh, while the previous version may be either in old or parent IndexWriter.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.