Skip to content

Conversation

@HyunSangHan
Copy link
Contributor

Description

Replaces the usage of Version class with RemoteVersion class in the reindex module's remote components to provide better isolation and backward compatibility for remote reindex operations.

Related Issues

Resolves #18391

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.

@HyunSangHan HyunSangHan requested a review from a team as a code owner June 6, 2025 16:27
@github-actions github-actions bot added enhancement Enhancement or improvement to existing feature or request good first issue Good for newcomers Other labels Jun 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 6, 2025

❌ Gradle check result for 03f243f: 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?

@andrross
Copy link
Member

andrross commented Jun 6, 2025

You've got a spotless code formatting failure here. You can fix it with ./gradlew :modules:reindex:spotlessApply.

Also note you can catch these failures locally by running ./gradlew precommit

@github-actions
Copy link
Contributor

github-actions bot commented Jun 7, 2025

❕ Gradle check result for fed080e: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@codecov
Copy link

codecov bot commented Jun 7, 2025

Codecov Report

❌ Patch coverage is 92.00000% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.85%. Comparing base (40e9e40) to head (2c68c88).
⚠️ Report is 299 commits behind head on main.

Files with missing lines Patch % Lines
...ch/index/reindex/remote/RemoteRequestBuilders.java 70.00% 0 Missing and 3 partials ⚠️
...ndex/reindex/remote/RemoteScrollableHitSource.java 0.00% 2 Missing ⚠️
...opensearch/index/reindex/remote/RemoteVersion.java 98.27% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #18457      +/-   ##
============================================
+ Coverage     72.80%   72.85%   +0.05%     
- Complexity    68609    68690      +81     
============================================
  Files          5572     5573       +1     
  Lines        315196   315254      +58     
  Branches      45750    45760      +10     
============================================
+ Hits         229466   229691     +225     
+ Misses        67166    67004     -162     
+ Partials      18564    18559       -5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2025

❌ Gradle check result for 5d31637: 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?

@HyunSangHan HyunSangHan closed this Jun 9, 2025
@HyunSangHan HyunSangHan reopened this Jun 9, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Jun 9, 2025

❕ Gradle check result for 5d31637: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@github-actions
Copy link
Contributor

❌ Gradle check result for b4ce564: 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
Copy link
Contributor

❕ Gradle check result for e823da8: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Jul 21, 2025
@HyunSangHan
Copy link
Contributor Author

@andrross @msfroh
Could you please review this when you are available?

Copy link
Contributor

@msfroh msfroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small callout on the comparator, but then I think this is ready to go. Thanks again, @HyunSangHan!

@HyunSangHan HyunSangHan requested a review from msfroh July 21, 2025 23:41
@github-actions
Copy link
Contributor

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

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Jul 23, 2025
@msfroh
Copy link
Contributor

msfroh commented Jul 24, 2025

Okay -- I think this just about looks ready to go, but we're still getting one integ test failure:

java.lang.AssertionError: 
Expected: <9.12.2>
     but: was <9.12.1>
	at __randomizedtesting.SeedInfo.seed([EE855C2C432D1442:63E061706425D70B]:0)
	at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
	at org.junit.Assert.assertThat(Assert.java:964)
	at org.junit.Assert.assertThat(Assert.java:930)
	at org.opensearch.qa.verify_version_constants.VerifyVersionConstantsIT.testLuceneVersionConstant(VerifyVersionConstantsIT.java:56)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1750)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:938)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:974)
	at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:988)

I think that's just a result of a Lucene update on the 2.19 branch. @HyunSangHan, can you please rebase onto the latest main branch and force-push the update?

@HyunSangHan
Copy link
Contributor Author

I think that's just a result of a Lucene update on the 2.19 branch. @HyunSangHan, can you please rebase onto the latest main branch and force-push the update?

@msfroh Thanks! I did right before now.

@github-actions
Copy link
Contributor

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

- Add RemoteVersion class with backward compatibility for Elasticsearch versions
- Support version parsing from string with distribution parameter
- Include version comparison methods (before, after, onOrAfter, onOrBefore)
- Support both Elasticsearch and OpenSearch version constants

This introduces a dedicated version class for remote reindex operations,
isolating it from the global Version class while maintaining full
backward compatibility with existing Elasticsearch clusters.

Signed-off-by: Hyunsang Han <[email protected]>
- Replace Version class usage with RemoteVersion in remote reindex components
- Update RemoteRequestBuilders to use RemoteVersion constants
- Update RemoteResponseParsers with improved distribution handling
- Update RemoteScrollableHitSource for version lookup compatibility

This completes the migration from global Version class to dedicated
RemoteVersion class in remote reindex functionality.

Signed-off-by: Hyunsang Han <[email protected]>
Simplifies the RemoteVersion class by replacing the distribution string field
with a boolean flag, making the code more readable and maintainable.

Signed-off-by: Hyunsang Han <[email protected]>
@github-actions
Copy link
Contributor

❌ Gradle check result for 2c68c88: 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?

@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label Aug 25, 2025
@HyunSangHan HyunSangHan reopened this Aug 25, 2025
@github-actions
Copy link
Contributor

❕ Gradle check result for 2c68c88: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label Aug 26, 2025
@HyunSangHan
Copy link
Contributor Author

Hi @msfroh,
Hope you’re doing well.
I’m ready to get your feedback whenever you have time.
Thank you!

@msfroh
Copy link
Contributor

msfroh commented Oct 9, 2025

Hey @HyunSangHan! Sorry for the delay, and thank you for tagging me.

I think this looks good and gets us even closer to cleaning up the Version class.

@msfroh msfroh merged commit af2a8fc into opensearch-project:main Oct 9, 2025
63 of 65 checks passed
rgsriram pushed a commit to rgsriram/OpenSearch that referenced this pull request Oct 11, 2025
…ect#18457)

- Add RemoteVersion class with backward compatibility for Elasticsearch versions
- Support version parsing from string with distribution parameter
- Include version comparison methods (before, after, onOrAfter, onOrBefore)
- Support both Elasticsearch and OpenSearch version constants

---------

Signed-off-by: Hyunsang Han <[email protected]>
peteralfonsi pushed a commit to peteralfonsi/OpenSearch that referenced this pull request Oct 15, 2025
…ect#18457)

- Add RemoteVersion class with backward compatibility for Elasticsearch versions
- Support version parsing from string with distribution parameter
- Include version comparison methods (before, after, onOrAfter, onOrBefore)
- Support both Elasticsearch and OpenSearch version constants

---------

Signed-off-by: Hyunsang Han <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Enhancement or improvement to existing feature or request good first issue Good for newcomers Other skip-changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Cleanup] Modify RemoteScrollableHitSource and RemoteRequestBuilders to stop using the Version class

3 participants