Skip to content
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

[Backport 2.x] [Refactor] XContent base classes from xcontent to core library (#5902) #6470

Merged
merged 1 commit into from
Mar 28, 2023

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Feb 23, 2023

Backport 9b9158e from #5902

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

Codecov Report

Merging #6470 (1f50ed0) into 2.x (0fa4a61) will increase coverage by 0.01%.
The diff coverage is 33.33%.

📣 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              @@
##                2.x    #6470      +/-   ##
============================================
+ Coverage     70.36%   70.38%   +0.01%     
- Complexity    59197    59261      +64     
============================================
  Files          4796     4796              
  Lines        284227   284205      -22     
  Branches      41304    41305       +1     
============================================
+ Hits         200005   200046      +41     
+ Misses        67523    67443      -80     
- Partials      16699    16716      +17     
Impacted Files Coverage Δ
...ensearch/gradle/precommit/ThirdPartyAuditTask.java 0.00% <0.00%> (ø)
...ch/plugin/noop/action/bulk/RestNoopBulkAction.java 0.00% <ø> (ø)
...java/org/opensearch/client/GetAliasesResponse.java 88.75% <ø> (ø)
...main/java/org/opensearch/client/NodesResponse.java 0.00% <ø> (ø)
...ava/org/opensearch/client/NodesResponseHeader.java 0.00% <ø> (ø)
.../java/org/opensearch/client/RequestConverters.java 85.19% <ø> (-0.34%) ⬇️
...ava/org/opensearch/client/RestHighLevelClient.java 41.75% <ø> (ø)
...pensearch/client/cluster/RemoteConnectionInfo.java 75.00% <ø> (+5.00%) ⬆️
.../opensearch/client/cluster/RemoteInfoResponse.java 100.00% <ø> (ø)
...g/opensearch/client/core/AcknowledgedResponse.java 70.58% <ø> (ø)
... and 667 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@andrross
Copy link
Member

@nknize I would very much like to backport this to avoid major divergence between the branches, but I'm having a hard time squaring this with our compatibility guarantees within a major version. Classes like XContentBuilder seem to be used by all or nearly all plugins (including the example plugins) so it seems to be a defacto API even if it isn't labeled as such. The package move is a trivial change for all plugins, but I just want to make sure we've got buy-in from all the plugins to do this work, otherwise we may end up reverting this backport.

@nknize
Copy link
Collaborator Author

nknize commented Mar 4, 2023

... I just want to make sure we've got buy-in from all the plugins to do this work, otherwise we may end up reverting this backport.

That's fair! Maybe we give it a time limit for feedback so it doesn't get lost indefinitely?

@dblock
Copy link
Member

dblock commented Mar 8, 2023

I am afraid this will make lots of people unhappy, even if it's a trivial change. But maybe we can help them by preparing the change for them? @nknize you could use meta with https://github.com/opensearch-project/opensearch-plugins and do a bulk change and submit a PR into each repo. Like so: https://github.com/opensearch-project/project-meta#make-a-change-in-many-repos

@nknize
Copy link
Collaborator Author

nknize commented Mar 27, 2023

The following issues have been opened to alert plugins of the coming refactor:

  1. alerting: [Backport] XContent Refactor to 2.x alerting#837
  2. anomaly-detection: [Backport] XContent Refactor to 2.x anomaly-detection#846
  3. asynchronous-search: [Backport] XContent Refactor to 2.x asynchronous-search#261
  4. common-utils: [Backport] XContent Refactor to 2.x common-utils#391
  5. cross-cluster-replication: [Backport] XContent Refactor to 2.x cross-cluster-replication#756
  6. data-prepper: [Refactor] XContent from common to core namespace data-prepper#2404
  7. geospatial: [Backport] XContent Refactor to 2.x geospatial#249
  8. job-scheduler: [Backport] XContent Refactor to 2.x job-scheduler#350
  9. k-nn: [Backport] XContent Refactor to 2.x k-NN#821
  10. ml-commons: [Backport] XContent Refactor to 2.x ml-commons#828
  11. neural-search: [Refactor] xcontent from common to core namespace neural-search#146
  12. notifications: [Backport] XContent Refactor to 2.x notifications#645
  13. opensearch-migrations: [Refactor] XContent from common to core namespace opensearch-migrations#123
  14. opensearch-oci-object-storage: [Refactor] XContent from common to core namespace opensearch-oci-object-storage#44
  15. opensearch-plugins (meta): [Refactor] XContent from common to core namespace opensearch-plugins#212
  16. opensearch-sdk-java: [Backport] XContent Refactor to 2.x opensearch-sdk-java#614
  17. performance-analyzer: [Refactor] XContent from common to core namespace performance-analyzer#406
  18. performance-analyzer-rca: [Refactor] XContent from common to core namespace performance-analyzer-rca#309
  19. reporting: [Backport] XContent Refactor to 2.x reporting#677
  20. search-processor: [Backport] XContent Refactor to 2.x search-processor#120
  21. security: [Backport] XContent Refactor to 2.x security#2588
  22. security-analytics: [Refactor] XContent from common to core namespace security-analytics#382
  23. simple-schema: [Refactor] XContent from common to core namespace simple-schema#72
  24. spring-data-opensearch: [Refactor] XContent from common to core namespace spring-data-opensearch#116
  25. sql: [Backport] XContent Refactor to 2.x sql#1475

In the future going forward this will not be a required step of core maintainers. Downstream projects will take it upon themselves independently to catch refactors / breaking changes early and fix as a subproject team.

@nknize
Copy link
Collaborator Author

nknize commented Mar 27, 2023

Opened #6841 for tracking

@nknize
Copy link
Collaborator Author

nknize commented Mar 27, 2023

I'm going to give this until the evening then merge so downstream folks have the week to make their refactor changes in advance of feature freeze. @reta @andrross @saratvemulapalli are y'all good with this?

@reta
Copy link
Collaborator

reta commented Mar 27, 2023

I'm going to give this until the evening then merge so downstream folks have the week to make their refactor changes in advance of feature freeze. @reta @andrross @saratvemulapalli are y'all good with this?

👍 to me, I hope the migration for community plugin developers is going to be straightforward

@saratvemulapalli
Copy link
Member

saratvemulapalli commented Mar 27, 2023

Hey @saratvemulapalli, I have a follow up PR for refactoring StreamInput and StreamOutput. Did y'all want to take that for the protobuf stuff or do you want me to open it? Also, do you have any concerns from an SDK perspective w/ this backport for 2.x? I'm also happy to help refactor the sdk as a top level library in the libs directory. Let me know how you want to handle that.

Thanks @nknize. I've opened up an RFC for Protobuf[1].
We are at point where we will learn as we go (cc: @VachaShah).
We cannot really have dependencies to slow down velocity of OpenSearch.
Im in for getting in the SDK one place where dependencies consume and everything within server/ directory is restricted for consumption.
I think this is also a question for maintainers on the SDK. I've opened up an issue[2]. SDK should be our long term solution to solve this problem.

I dont think SDK has concerns, OpenSearch 2.x is SDK 1.x and we should just backport a PR which shouldn't be a problem. Tagging @dbwiddis for thoughts.

[1] #6844
[2] opensearch-project/opensearch-sdk-java#616

@saratvemulapalli
Copy link
Member

I'm going to give this until the evening then merge so downstream folks have the week to make their refactor changes in advance of feature freeze. @reta @andrross @saratvemulapalli are y'all good with this?

👍🏻 . Plugins already have consumed these changes in 3.0 (a.k.a main) should be a backport to their 2.x branch, it shouldn't be invasive.

I am worried for plugins externally which are not part of the project since its a breaking change, communicating is the best thing we could do.

@dblock
Copy link
Member

dblock commented May 29, 2023

Here's one: #7728

@nknize nknize added the backport PRs or issues specific to backporting features or enhancments label Jun 28, 2023
@AndreVirtimo
Copy link

Why is this not mentioned in the changelog? This was a breaking change!

@dblock
Copy link
Member

dblock commented Jul 6, 2023

It should have been mentioned in the CHANGELOG. @nknize can you please add it?

Regarding whether this was a breaking change in the semver sense, it's not. You can take a deep dependency on just about anything in core, and any change will break anyone that takes such a dependency, including a major upgrade of a dependent library. We ensure that all end-user features (e.g. RESTful interfaces) follow semver, but for java dependencies OpenSearch requires you to rebuild your plugin against every minor version.

To solve this in core, @nknize is refactoring such that we can one day declare that org.opensearch.core.* follows semver.
To solve this for clients, we have opensearch-project/opensearch-java#262.
To solve this for plugins, we have https://github.com/opensearch-project/opensearch-sdk-java.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport PRs or issues specific to backporting features or enhancments >breaking Identifies a breaking change. skip-changelog stalled Issues that have stalled v2.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants