-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Deprecate Map, List, and Set in org.opensearch.common.collect (#6609)… #6687
Deprecate Map, List, and Set in org.opensearch.common.collect (#6609)… #6687
Conversation
…arch-project#6609) (opensearch-project#6631) * Deprecate Map, List, and Set in org.opensearch.common.collect (opensearch-project#6609) Signed-off-by: Dilip Roshitha <[email protected]> * Deprecate Map, List, and Set in org.opensearch.common.collect - PR review points (opensearch-project#6609) Signed-off-by: Dilip Roshitha <[email protected]> * Deprecate Map, List, and Set in org.opensearch.common.collect - removed associated tests (opensearch-project#6609) Signed-off-by: Dilip Roshitha <[email protected]> --------- Signed-off-by: Dilip Roshitha <[email protected]> (cherry picked from commit defcdf9)
The fix for the whitesource issue is at: #6629 |
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #6687 +/- ##
============================================
- Coverage 70.80% 70.78% -0.02%
+ Complexity 59219 59216 -3
============================================
Files 4808 4808
Lines 283454 283440 -14
Branches 40868 40868
============================================
- Hits 200691 200625 -66
- Misses 66268 66353 +85
+ Partials 16495 16462 -33
... and 475 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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 is great, I like us moving away from custom pieces in OpenSearch :)
@@ -97,6 +97,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
- [Refactor] XContent base classes from xcontent to core library ([#5902](https://github.com/opensearch-project/OpenSearch/pull/5902)) | |||
|
|||
### Deprecated | |||
- Map, List, and Set in org.opensearch.common.collect ([#6609](https://github.com/opensearch-project/OpenSearch/pull/6609)) |
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 will be an invasive change across the project. Most plugins use these data structures[1].
I would take this opportunity to remove these implementations completely in 3.0 and have these deprecations in 2.x. Since we have the flexibility to make breaking changes in a new major. WDYT @reta @andrross @nknize
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.
@saratvemulapalli it makes sense but the only concern I have is how far are we from 3.0.0 release? We have to give people (plugin developers primarily) enough time to move off the deprecated APIs, if 3.0.0 is year from now - this is fine to deprecate + remove, if it is just few months - we should postpone removal to 4.0.0 I think.
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.
These deprecations have already been merged as a part of 2.x
. I am trying to get main in sync with those changes.
We can have a campaign for removal as a part of 3.0.0
, if necessary. But I still think we need to merge this in to get the branches in sync first.
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.
@reta from release schedule[1], looks like we are atleast 9-11 months away from 3.0.
@kotwanikunal absolutely, lets get this merged.
If all 3/other maintainers agree, could you create a campaign that these data structures will be removed?
Please Vote 👍🏻 👎🏻
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.
I will keep a track on the votes, but as per the current majority, I will create a campaign soon for 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.
Campaign: #6786
…arch-project#6609) (opensearch-project#6631) (opensearch-project#6687) * Deprecate Map, List, and Set in org.opensearch.common.collect (opensearch-project#6609) Signed-off-by: Dilip Roshitha <[email protected]> * Deprecate Map, List, and Set in org.opensearch.common.collect - PR review points (opensearch-project#6609) Signed-off-by: Dilip Roshitha <[email protected]> * Deprecate Map, List, and Set in org.opensearch.common.collect - removed associated tests (opensearch-project#6609) Signed-off-by: Dilip Roshitha <[email protected]> --------- Signed-off-by: Dilip Roshitha <[email protected]> (cherry picked from commit defcdf9) Co-authored-by: Dilip Roshitha <[email protected]> Signed-off-by: Mingshi Liu <[email protected]>
… (#6631)
Signed-off-by: Dilip Roshitha [email protected]
Signed-off-by: Dilip Roshitha [email protected]
Signed-off-by: Dilip Roshitha [email protected]
Signed-off-by: Dilip Roshitha [email protected]
(cherry picked from commit defcdf9)
Description
main
Issues Resolved
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.