Created new metadata lib and added AliasMetadataModel and MappingMetadataModel classes#20328
Created new metadata lib and added AliasMetadataModel and MappingMetadataModel classes#20328gargmanik13 wants to merge 1 commit intoopensearch-project:mainfrom
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 Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR introduces a new metadata library ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
Cluster Manager
enhancement
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
from
e41bf9e to
aa3ee56
Compare
|
❌ Gradle check result for aa3ee56: 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? |
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
from
aa3ee56 to
47c27ce
Compare
|
❌ Gradle check result for 47c27ce: 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? |
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
from
47c27ce to
1bc5838
Compare
|
❌ Gradle check result for 1bc5838: 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? |
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
from
1bc5838 to
5318259
Compare
|
❌ Gradle check result for 5318259: 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? |
Test Result (2 failures / +1) Flaky tests: |
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
from
5318259 to
31df1c5
Compare
|
❌ Gradle check result for 31df1c5: 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? |
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
from
31df1c5 to
313a39a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20328 +/- ##
============================================
- Coverage 73.30% 73.27% -0.03%
- Complexity 71965 72033 +68
============================================
Files 5781 5785 +4
Lines 329200 329363 +163
Branches 47491 47492 +1
============================================
+ Hits 241314 241348 +34
- Misses 68491 68683 +192
+ Partials 19395 19332 -63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@libs/metadata/src/main/java/org/opensearch/metadata/compress/CompressedData.java`:
- Around line 37-75: The class exposes its internal byte[] allowing external
mutation; update the two constructors (CompressedData(byte[] compressedBytes,
int checksum) and CompressedData(StreamInput in)) to null-guard and store a
defensive copy (e.g., Arrays.copyOf) of the incoming byte[]; update
compressedBytes() to return a fresh copy instead of the internal array; ensure
writeTo uses the internal array (already safe) but if you choose to accept null
inputs, normalize to an empty byte[] on construction so writeTo and checksum
logic never see null. Use Arrays.copyOf to perform copies.
In
`@libs/metadata/src/main/java/org/opensearch/metadata/index/model/MappingMetadataModel.java`:
- Around line 40-44: The constructor MappingMetadataModel(String type,
CompressedData source, boolean routingRequired) allows a null source but
writeTo(OutputStream/StreamOutput) calls source.writeTo(out), causing an NPE;
either validate and reject null in the constructor (throw
NPE/IllegalArgumentException) or treat null as a valid state and update writeTo
to handle a null source (e.g., write a boolean flag then write source only if
present) and mirror the change in the deserialization constructor/readFrom so
reading and writing remain consistent; update the relevant symbols:
MappingMetadataModel constructor, writeTo(...), and the deserialization
constructor/readFrom to keep serialization symmetric.
In `@server/src/main/java/org/opensearch/cluster/metadata/AliasMetadata.java`:
- Around line 160-166: The new AliasMetadataModel.writeTo()/constructor changed
the wire format for indexRouting/searchRouting by switching to
writeOptionalString()/readOptionalString(), breaking compatibility with older
nodes that expect a boolean marker + conditional writeString/readString; restore
the original serialization pattern (in AliasMetadataModel.writeTo and
AliasMetadataModel(StreamInput)) by writing a boolean marker before each routing
string and conditionally writing/reading the string, or implement explicit
version-gated handling in AliasMetadata.writeTo and AliasMetadata(StreamInput)
that emits/consumes the old boolean+string format for older stream versions and
the new optional-string format for newer versions, ensuring indexRouting and
searchRouting are encoded exactly as the old nodes expect when the
StreamInput/StreamOutput version indicates older clusters.
In `@server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java`:
- Around line 162-165: Add explicit cross-version serialization guards and/or
rolling-upgrade tests: update MappingMetadata.writeTo (and the corresponding
deserialization constructor that reads type, source checksum+bytes, and
routingRequired) to include version checks using the StreamOutput/StreamInput
version APIs and branch serialization accordingly, and add unit/integration
tests that simulate older cluster node versions to validate compatibility during
rolling upgrades; ensure model.writeTo remains unchanged except behind the same
version guard so the read/write order stays compatible when communicating with
older versions.
🧹 Nitpick comments (6)
libs/metadata/build.gradle (1)
20-24: Avoid blanket disabling of audit tasks if possible.Consider re-enabling these checks and adding targeted exclusions instead, or document the rationale to keep compliance posture clear.
libs/metadata/src/test/java/org/opensearch/metadata/compress/CompressedDataTests.java (1)
27-32: Consider renaming the helper method to avoid shadowing.The private
assertEqualsmethod shadows the inheritedassertEqualsfrom the test framework. While it works correctly here, this could cause confusion when reading the code or if someone adds assertions expecting JUnit's behavior.Suggested rename
- private void assertEquals(CompressedData d1, CompressedData d2) { + private void assertCompressedDataEquals(CompressedData d1, CompressedData d2) { assertThat(d1, equalTo(d2)); assertArrayEquals(d1.compressedBytes(), d2.compressedBytes()); assertThat(d1.checksum(), equalTo(d2.checksum())); assertThat(d1.hashCode(), equalTo(d2.hashCode())); }Then update call sites at lines 46, 58, 86, and 105 accordingly.
libs/metadata/src/main/java/org/opensearch/metadata/index/model/AliasMetadataModel.java (2)
54-69: Consider adding null validation for thealiasfield.The
aliasfield is required (non-nullable per the Javadoc at line 118 stating "never null"), but the constructor doesn't validate this. A null alias could cause issues downstream.Suggested fix
public AliasMetadataModel( String alias, CompressedData filter, String indexRouting, String searchRouting, `@Nullable` Boolean writeIndex, `@Nullable` Boolean isHidden ) { + if (alias == null) { + throw new IllegalArgumentException("alias must not be null"); + } this.alias = alias; this.filter = filter;
265-267: Add null validation in Builder constructor.Consistent with the main constructor, the Builder should validate that
aliasis not null.Suggested fix
public Builder(String alias) { + if (alias == null) { + throw new IllegalArgumentException("alias must not be null"); + } this.alias = alias; }libs/metadata/src/test/java/org/opensearch/metadata/index/model/MappingMetadataModelTests.java (1)
24-61: LGTM, but consider adding inequality tests.The serialization and equality tests are good. For completeness with
CompressedDataTests, consider adding tests for:
testNotEqualswith different type, source, or routingRequiredtestHashCodeconsistencyserver/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java (1)
135-137: Consider caching theCompressedXContentinstance.The
source()method creates a newCompressedXContentwrapper on each call. If this method is called frequently, consider caching the result lazily.Suggested optimization
+ private volatile CompressedXContent sourceCache; + public CompressedXContent source() { - return new CompressedXContent(model.source()); + CompressedXContent result = sourceCache; + if (result == null) { + result = new CompressedXContent(model.source()); + sourceCache = result; + } + return result; }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
CHANGELOG.mdlibs/metadata/build.gradlelibs/metadata/src/main/java/org/opensearch/metadata/compress/CompressedData.javalibs/metadata/src/main/java/org/opensearch/metadata/compress/package-info.javalibs/metadata/src/main/java/org/opensearch/metadata/index/model/AliasMetadataModel.javalibs/metadata/src/main/java/org/opensearch/metadata/index/model/MappingMetadataModel.javalibs/metadata/src/main/java/org/opensearch/metadata/index/model/package-info.javalibs/metadata/src/test/java/org/opensearch/metadata/compress/CompressedDataTests.javalibs/metadata/src/test/java/org/opensearch/metadata/index/model/AliasMetadataModelTests.javalibs/metadata/src/test/java/org/opensearch/metadata/index/model/MappingMetadataModelTests.javaserver/build.gradleserver/src/main/java/org/opensearch/cluster/metadata/AliasMetadata.javaserver/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.javaserver/src/main/java/org/opensearch/common/compress/CompressedXContent.javaserver/src/test/java/org/opensearch/cluster/metadata/AliasMetadataTests.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-01-02T19:23:29.698Z
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:29.698Z
Learning: The gRPC search API in OpenSearch is marked as "experimental" in official documentation, so changes to proto schemas that remove previously unsupported fields (those throwing UnsupportedOperationException) are not considered breaking changes.
Applied to files:
CHANGELOG.md
📚 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/cluster/metadata/MappingMetadata.javaserver/src/main/java/org/opensearch/common/compress/CompressedXContent.javaserver/src/main/java/org/opensearch/cluster/metadata/AliasMetadata.java
🧬 Code graph analysis (2)
libs/metadata/src/test/java/org/opensearch/metadata/index/model/AliasMetadataModelTests.java (1)
libs/common/src/main/java/org/opensearch/common/util/set/Sets.java (1)
Sets(57-185)
libs/metadata/src/main/java/org/opensearch/metadata/index/model/AliasMetadataModel.java (3)
libs/metadata/src/main/java/org/opensearch/metadata/compress/CompressedData.java (1)
ExperimentalApi(25-90)libs/metadata/src/main/java/org/opensearch/metadata/index/model/MappingMetadataModel.java (1)
ExperimentalApi(26-126)libs/common/src/main/java/org/opensearch/common/util/set/Sets.java (1)
Sets(57-185)
🔇 Additional comments (16)
libs/metadata/src/main/java/org/opensearch/metadata/index/model/package-info.java (1)
1-10: LGTM — clear package-level documentation.CHANGELOG.md (1)
16-16: Changelog entry looks good.Clear and aligned with the PR scope.
libs/metadata/src/main/java/org/opensearch/metadata/compress/package-info.java (1)
1-10: LGTM.Standard license header and package declaration.
server/build.gradle (1)
75-75: Dependency addition looks good.Integrates the new metadata library as intended.
libs/metadata/src/test/java/org/opensearch/metadata/index/model/AliasMetadataModelTests.java (1)
27-36: The test expectations are correct and consistent with legacy behavior. BothAliasMetadataandAliasMetadataModeluseStrings.splitStringByCommaToArray(), which internally calls Java'sString.split(","). This method preserves whitespace—it splits only on commas. The test inAliasMetadataModelTests.javaline 36 correctly expects{"trim", "tw ", " ltw ", " lw"}, matching the identical assertion inAliasMetadataTests.javaline 60. Additionally,AliasMetadataTests.javalines 91–92 explicitly validate thatmodel.searchRoutingValues()equalsaliasMetadata.searchRoutingValues(), confirming the model aligns with legacy behavior. No divergence exists.Likely an incorrect or invalid review comment.
libs/metadata/src/test/java/org/opensearch/metadata/compress/CompressedDataTests.java (1)
34-128: LGTM!The test coverage is comprehensive, including serialization round-trips, equality/hashCode contracts, and randomized stress testing. The tests follow OpenSearch testing patterns well.
server/src/main/java/org/opensearch/common/compress/CompressedXContent.java (3)
79-88: LGTM on the refactoring to use CompressedData.The field change from separate
bytes/crc32to a singleCompressedDatainstance is clean. The new constructor properly callsassertConsistent()to validate state.
170-177: Verify the mutability implications of the new accessor.The new
compressedData()method exposes the internalCompressedData, and perCompressedData.compressedBytes()implementation, it returns the raw byte array without a defensive copy. This could allow callers to mutate the internal state.Since
CompressedXContentis@PublicApi(since = "1.0.0")and the class isfinal, consider whether theCompressedDatashould make a defensive copy incompressedBytes(), or document the expectation that callers must not mutate the returned array.
179-186: LGTM on serialization delegation.The read/write paths properly delegate to
CompressedData. The serialization format (checksum then bytes) is preserved throughCompressedData's implementation.libs/metadata/src/main/java/org/opensearch/metadata/index/model/AliasMetadataModel.java (2)
87-99: LGTM on serialization format.The serialization order (alias, filter presence/data, indexRouting, searchRouting, writeIndex, isHidden) is consistent between read and write operations. The use of optional methods for nullable fields follows OpenSearch conventions.
Also applies to: 198-211
220-244: LGTM on equals/hashCode implementation.Correctly excludes the derived
searchRoutingValuesfield since it's computed fromsearchRouting. The implementation properly usesObjects.equalsandObjects.hashfor null-safe comparisons.libs/metadata/src/main/java/org/opensearch/metadata/index/model/MappingMetadataModel.java (1)
52-56: LGTM on deserialization constructor.The deserialization order matches
writeTo(): type, source, routingRequired.server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java (2)
167-172: LGTM onwriteVerifiableToimplementation.This method intentionally doesn't delegate to the model because it needs to write through
BufferedChecksumStreamOutputfor verification purposes, including callingsource().writeVerifiableTo(out)for the compressed content's checksum.
69-106: LGTM on constructor refactoring.All constructors properly initialize the
MappingMetadataModelwith the same data that was previously stored directly. The routing extraction logic inisRoutingRequiredremains unchanged.server/src/test/java/org/opensearch/cluster/metadata/AliasMetadataTests.java (1)
40-40: Good coverage for model deserialization.The new tests validate both populated and minimal AliasMetadata cases and should guard compatibility well.
Also applies to: 71-126
server/src/main/java/org/opensearch/cluster/metadata/AliasMetadata.java (1)
52-127: Refactor keeps accessor/builder behavior intact.The delegation preserves the public accessors, builder flow, and XContent output semantics, including null handling for filter/routing/writeIndex/isHidden.
Also applies to: 151-157, 191-259, 268-285
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
libs/metadata/src/main/java/org/opensearch/metadata/compress/CompressedData.java
Show resolved
Hide resolved
libs/metadata/src/main/java/org/opensearch/metadata/index/model/MappingMetadataModel.java
Show resolved
Hide resolved
server/src/main/java/org/opensearch/cluster/metadata/MappingMetadata.java
Show resolved
Hide resolved
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
2 times, most recently
from
76c92c6 to
faa4ee1
Compare
…Model Signed-off-by: Manik Garg <gargmanik1317@gmail.com>
gargmanik13
force-pushed
the
md_lib_stateless_cm
branch
from
faa4ee1 to
df986e0
Compare
2 participants
Description
Related Issues
Resolves #13197
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.