Replace LegacyESVersion.fromString with Version.fromString

[quangdutran]

Description

Replace LegacyESVersion.fromString with Version.fromString

Related Issues

Resolves #18392

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 4dbba59: 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?

[sandeshkr419]

REMOTE_CLUSTER_CLIENT_ROLE_VERSION and LEGACY_ROLES (L354) are not used in the code base. Let's remove these variables entirely.

[sandeshkr419]

Thanks @quangdutran for the changes. In DiscoveryNodeRole.java, the usages are probably dead-code and you can safely delete those consumers of LegacyESVersion usage.

In test classes, I don't have any comments - LGTM.

One more thing you should do is to delete the class LegacyESVersion.java which I assume should not have any usage in the package. Given its not a publicly exposed class, it should be safe to delete without any issues. Deleting it will also ensure that we didn't missed any usage of it.

[msfroh]

One more thing you should do is to delete the class LegacyESVersion.java which I assume should not have any usage in the package. Given its not a publicly exposed class, it should be safe to delete without any issues. Deleting it will also ensure that we didn't missed any usage of it.

There's a meta parent task #18387, which covers the work that needs to be done to safely remove LegacyESVersion. There are a few more PRs that need to be merged first.

[sandeshkr419]

Thanks @msfroh for the clarification.

In that case, @quangdutran please address the comment on DiscoveryNodeRole.java and get the CI to pass - rest everything LGTM!

[github-actions]

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

[quangdutran]

Hi @sandeshkr419

I don't know why the test is kinda flaky and random. I tried gradle check locally and it did not report any failure but the pipeline just fails with random tests, which are not really related to my change IMO.

Latest fail because of org.opensearch.indexing.IndexActionIT.testAutoGenerateIdNoDuplicates

Could you give me an advice on this?

[msfroh]

Latest fail because of org.opensearch.indexing.IndexActionIT.testAutoGenerateIdNoDuplicates

Could you give me an advice on this?

Unfortunately, we do have some tests that fail randomly. I just retriggered the test run to see if we have better luck next time.

It looks like at some point, VerifyVersionConstantsIT failed. If that one fails again, we definitely want to look into it.

Incidentally, specific line that's failing on IndexActionIT shouldn't even be there. Support for "types" within an index was removed in time for OpenSearch 2.0. That test was checking without specifying a type and with a type specified. Since types were removed, the test has been running searches without types twice. We can remove the second one. What's interesting is that the second check failed -- even though it's doing exactly the same thing as the first one, which succeeded. (Of course, it's also supposed to run the checks in a loop, so clearly at some point there was concern about the doc count changing between searches.) Anyway, I'm going to open a quick PR to clean up the redundant test: #18630

[github-actions]

✅ Gradle check result for e1122c0: SUCCESS

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.75%. Comparing base (0874e58) to head (74fce13).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #18567      +/-   ##
============================================
- Coverage     72.80%   72.75%   -0.05%     
+ Complexity    68437    68397      -40     
============================================
  Files          5563     5563              
  Lines        314174   314172       -2     
  Branches      45554    45554              
============================================
- Hits         228726   228583     -143     
- Misses        66871    67019     +148     
+ Partials      18577    18570       -7     

☔ 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.

[msfroh]

The detect breaking changes workflow is failing with:

***! MODIFIED CLASS: PUBLIC ABSTRACT org.opensearch.cluster.node.DiscoveryNodeRole  (not serializable)
	===  CLASS FILE FORMAT VERSION: 65.0 <- 65.0
	---! REMOVED FIELD: PUBLIC(-) STATIC(-) FINAL(-) org.opensearch.Version REMOTE_CLUSTER_CLIENT_ROLE_VERSION

We know that REMOTE_CLUSTER_CLIENT_ROLE_VERSION is not used anywhere in opensearch-project (ref). In theory, some hypothetical 3rd party plugin could reference it, but I find it difficult to even entertain that possibility.

So, do we add it back to make our tools happy and only remove it in 4.0? Maybe. At the very least, we need to do a better job of flagging the things that should be removed in the next major version. We clearly didn't remove enough stuff in 2.0 or 3.0 (since this should have been removed in 2.0).

I really disagree with the API BWC policy that we've decided to enforce, but of course, I'm approaching it from the perspective of a core developer whose hands are tied, rather than from the perspective of a plugin developer who potentially experiences toil when core APIs change.

[msfroh]

Tl;dr: I hate this, but I think we need to add back that stupid, useless, unused field in order to make our tools happy.

[github-actions]

❌ Gradle check result for 74fce13: 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 74fce13: SUCCESS