Improve exception messaging when encountering legacy version IDs

[msfroh]

Description

When trying to upgrade an index that was originally created on Elasticsearch, we should throw a human-readable exception saying that it's not supported, rather than a cryptic message like "Version id 7090199 must contain OpenSearch mask".

Related Issues

Resolves #20499

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]

❌ Gradle check result for 4407508: 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-actions]

❌ Gradle check result for 7f39545: 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-actions]

❌ Gradle check result for 7f39545: 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]

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

The PR improves error messaging when opening OpenSearch 3.x indices originally created with Elasticsearch. It introduces a new UnsupportedVersionException in the Version class and enhances error handling in IndexMetadata to replace generic "must contain OpenSearch mask" messages with human-readable Elasticsearch version identifiers.

Changes

Cohort / File(s)	Summary
Version API Enhancement libs/core/src/main/java/org/opensearch/Version.java	Adds UnsupportedVersionException inner class, introduces legacyFriendlyIdToString(int) helper for human-readable legacy version formatting, replaces IllegalArgumentException with UnsupportedVersionException in fromId(), and replaces stringHasLength() usage with Strings.hasLength() from core commons.
Validation Refactoring libs/core/src/main/java/org/opensearch/semver/SemverRange.java	Replaces Version.stringHasLength() call with Strings.hasLength() for emptiness checking, maintaining existing exception behavior.
Error Handling server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java	Wraps indexCreated() version retrieval in try-catch to handle UnsupportedVersionException and throw IllegalArgumentException with detailed context. Minor javadoc clarifications for public methods.
Test Coverage server/src/test/java/org/opensearch/VersionTests.java, server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java	Adds testLegacyVersion() verifying UnsupportedVersionException is thrown with correct version string formatting, and testLegacyCreatedVersion() verifying exception propagation through index metadata construction.
Documentation CHANGELOG.md	Documents improved error messaging for unsupported index versions in Unreleased 3.x section.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Fix a flaky test for issue 19915  #20253: Updates test utilities to generate masked/valid version IDs to avoid failures from stricter Version.fromId() validation introduced by this change.

Suggested labels

v3.4.0, backport 3.4, Indexing:Replication

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 70.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and accurately summarizes the main objective of the PR—improving exception messaging for legacy version IDs.
Description check	✅ Passed	The description provides a clear problem statement and references the linked issue, with testing confirmed. Most template sections are addressed appropriately.
Linked Issues check	✅ Passed	The code changes fully address issue #20499 by replacing the cryptic 'OpenSearch mask' error with a human-readable exception for legacy version IDs [#20499].
Out of Scope Changes check	✅ Passed	All changes are directly related to improving exception messaging for legacy version IDs and their documentation. No unrelated modifications detected.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

Tip

Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

❌ Gradle check result for 9a1a353: 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]

Looks good to me! The failures look unrelated, retried gradle check

[github-actions]

❌ Gradle check result for 9a1a353: 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?

[msfroh]

Hmm... the latest build failed with:

ERROR][o.o.g.r.RemoteClusterStateService] [v2.19.5-remote-2] Failed to read cluster state from remote
»  org.opensearch.gateway.remote.RemoteStateTransferException: Download failed for nodes
»  	at org.opensearch.gateway.remote.RemoteClusterStateAttributesManager.lambda$getWrappedReadListener$3(RemoteClusterStateAttributesManager.java:103)
»  	at org.opensearch.core.action.ActionListener$1.onFailure(ActionListener.java:90)
»  	at org.opensearch.common.remote.RemoteWriteableEntityBlobStore.lambda$readAsync$0(RemoteWriteableEntityBlobStore.java:87)
»  	at org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:955)
»  	at java.****/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
»  	at java.****/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
»  	at java.****/java.lang.Thread.run(Thread.java:1583)
»  Caused by: java.lang.IllegalStateException: unexpected byte [0x1d]
»  	at org.opensearch.core.common.io.stream.StreamInput.readBoolean(StreamInput.java:596)
»  	at org.opensearch.core.common.io.stream.StreamInput.readBoolean(StreamInput.java:586)
»  	at org.opensearch.cluster.node.DiscoveryNode.<init>(DiscoveryNode.java:344)
»  	at org.opensearch.cluster.node.DiscoveryNodes.readFrom(DiscoveryNodes.java:777)
»  	at org.opensearch.gateway.remote.model.RemoteDiscoveryNodes.lambda$static$0(RemoteDiscoveryNodes.java:37)
»  	at org.opensearch.repositories.blobstore.ChecksumWritableBlobStoreFormat.deserialize(ChecksumWritableBlobStoreFormat.java:105)
»  	at org.opensearch.gateway.remote.model.RemoteDiscoveryNodes.deserialize(RemoteDiscoveryNodes.java:101)
»  	at org.opensearch.gateway.remote.model.RemoteDiscoveryNodes.deserialize(RemoteDiscoveryNodes.java:32)
»  	at org.opensearch.common.remote.RemoteWriteableEntityBlobStore.read(RemoteWriteableEntityBlobStore.java:77)
»  	at org.opensearch.common.remote.RemoteWriteableEntityBlobStore.lambda$readAsync$0(RemoteWriteableEntityBlobStore.java:85)
»  	... 4 more

Checking the 2.19.5 code, that's this line.

[github-actions]

❌ Gradle check result for 2e98d06: 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-actions]

❌ Gradle check result for 2e98d06: 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-actions]

❌ Gradle check result for 4e38446: 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-actions]

PR Reviewer Guide 🔍

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: Refactor Version utility methods and add UnsupportedVersionException Relevant files: libs/core/src/main/java/org/opensearch/Version.java libs/core/src/main/java/org/opensearch/semver/SemverRange.java server/src/test/java/org/opensearch/VersionTests.java Sub-PR theme: Improve error messaging for legacy ES index creation version in IndexMetadata Relevant files: server/src/main/java/org/opensearch/cluster/metadata/IndexMetadata.java server/src/test/java/org/opensearch/cluster/metadata/IndexMetadataTests.java

⚡ Recommended focus areas for review Double Computation In UnsupportedVersionException, legacyFriendlyIdToString(versionId) is called twice: once in the super() call and once to assign versionString. This is redundant and could be simplified by computing the string once and reusing it. private UnsupportedVersionException(int versionId) { super(String.format(Locale.ROOT, "Unsupported version [%s]", legacyFriendlyIdToString(versionId))); this.versionString = legacyFriendlyIdToString(versionId); } Constants Order The legacyFriendlyIdToString method references MASK, MAJOR_SHIFT, MINOR_SHIFT, REVISION_SHIFT, and VERSION_SHIFT constants that are defined after the method (lines 91-94). In Java, static fields are initialized in order, but since this is a static method (not a field initializer), this is fine at runtime. However, the ordering may be confusing to readers and could be considered a style issue. private static String legacyFriendlyIdToString(int versionId) { String prefix; if ((versionId & MASK) != 0) { versionId = versionId ^ MASK; prefix = ""; } else { prefix = "ES "; } int major = (versionId / MAJOR_SHIFT) % VERSION_SHIFT; int minor = (versionId / MINOR_SHIFT) % VERSION_SHIFT; int revision = (versionId / REVISION_SHIFT) % VERSION_SHIFT; return prefix + major + "." + minor + "." + revision; } Exception Wrapping In indexCreated, the UnsupportedVersionException is caught and re-thrown as an IllegalArgumentException. Callers catching IllegalArgumentException may not realize the root cause is a legacy ES version. Consider whether it would be better to propagate UnsupportedVersionException directly (since it's a RuntimeException) to preserve the specific exception type for callers who want to handle it distinctly. try { indexVersion = SETTING_INDEX_VERSION_CREATED.get(indexSettings); } catch (Version.UnsupportedVersionException e) { final String message = String.format( Locale.ROOT, "index with UUID [%s] created on version [%s] is not supported by version [%s]", indexSettings.get(IndexMetadata.SETTING_INDEX_UUID), e.getVersionString(), Version.CURRENT.toString() ); throw new IllegalArgumentException(message, e); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Avoid redundant method calls in constructor legacyFriendlyIdToString(versionId) is called twice in the constructor — once for the message and once to set versionString. Since the method modifies the local versionId parameter internally, the two calls are safe, but it's still redundant. Compute the value once and reuse it to avoid the double computation. libs/core/src/main/java/org/opensearch/Version.java [66-69] private UnsupportedVersionException(int versionId) { - super(String.format(Locale.ROOT, "Unsupported version [%s]", legacyFriendlyIdToString(versionId))); - this.versionString = legacyFriendlyIdToString(versionId); + this(legacyFriendlyIdToString(versionId)); } +private UnsupportedVersionException(String versionString) { + super(String.format(Locale.ROOT, "Unsupported version [%s]", versionString)); + this.versionString = versionString; +} + Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that legacyFriendlyIdToString(versionId) is called twice in the constructor, which is a minor inefficiency. Computing the value once and reusing it is a valid improvement, though the performance impact is negligible.	Low
Possible issue	Fix forward reference to uninitialized constants The legacyFriendlyIdToString method is defined before the constants (MASK, MAJOR_SHIFT, etc.) it references, which will cause a compilation error since static fields are initialized in order. Move the method after the constant declarations, or move the constants before the method. libs/core/src/main/java/org/opensearch/Version.java [76-94] +private static final int VERSION_SHIFT = 100; // Two digits per version part +private static final int REVISION_SHIFT = VERSION_SHIFT; +private static final int MINOR_SHIFT = VERSION_SHIFT * VERSION_SHIFT; +private static final int MAJOR_SHIFT = VERSION_SHIFT * VERSION_SHIFT * VERSION_SHIFT; +// ... other constants including MASK ... + private static String legacyFriendlyIdToString(int versionId) { String prefix; if ((versionId & MASK) != 0) { versionId = versionId ^ MASK; prefix = ""; } else { prefix = "ES "; } int major = (versionId / MAJOR_SHIFT) % VERSION_SHIFT; int minor = (versionId / MINOR_SHIFT) % VERSION_SHIFT; int revision = (versionId / REVISION_SHIFT) % VERSION_SHIFT; return prefix + major + "." + minor + "." + revision; } -private static final int VERSION_SHIFT = 100; - Suggestion importance[1-10]: 1 __ Why: In Java, methods can reference static fields regardless of declaration order — static fields are initialized before any method is called, so there is no compilation error here. The suggestion is based on a misunderstanding of Java's static initialization rules; methods are not subject to forward-reference restrictions like static field initializers are.	Low

[github-actions]

❌ Gradle check result for 334576f: 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?