-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added version-specific OpenSearchWorkCoordinators #1226
Conversation
Signed-off-by: Chris Helma <[email protected]>
...entsFromSnapshotMigration/src/test/java/org/opensearch/migrations/bulkload/EndToEndTest.java
Outdated
Show resolved
Hide resolved
...main/java/org/opensearch/migrations/bulkload/workcoordination/OpenSearchWorkCoordinator.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Chris Helma <[email protected]>
@@ -181,6 +181,12 @@ public static class Args { | |||
description = ("Version of the source cluster.")) | |||
public Version sourceVersion = Version.fromString("ES 7.10"); | |||
|
|||
@Parameter(required = true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Target version detection build into the OpenSearch client, can we use that instead of requiring a parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Easy to do, thanks for the idea! I'd forgotten about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't seem right, 6.8 is not an experimental target, if we are only supporting using RFS for 6.8 -> 6.8 migration, then we should only update the specific scenarios that we want for its verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aren't officially supporting ES 6.8 as a target, it's being added as an un-announced feature for a specific user. Experimental seems like an appropriate adjective to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about it a bit more and think I better understand your point - we shouldn't have 6.8 in this class at all and just update the tests to cover it. Makes sense, will address.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
Signed-off-by: Chris Helma <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## subshard #1226 +/- ##
==============================================
- Coverage 80.30% 80.27% -0.04%
- Complexity 3101 3114 +13
==============================================
Files 423 425 +2
Lines 15850 15880 +30
Branches 1072 1072
==============================================
+ Hits 12728 12747 +19
- Misses 2474 2483 +9
- Partials 648 650 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Chris Helma <[email protected]>
Description
Issues Resolved
Testing
Ran the unit tests
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.