-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[Refactor] XContent base classes from xcontent to core library #5902
[Refactor] XContent base classes from xcontent to core library #5902
Conversation
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
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 #5902 +/- ##
============================================
- Coverage 70.70% 70.61% -0.09%
+ Complexity 58869 58765 -104
============================================
Files 4789 4789
Lines 281788 281783 -5
Branches 40669 40669
============================================
- Hits 199229 198972 -257
- Misses 66227 66436 +209
- Partials 16332 16375 +43
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Am I correct in assuming that the Java package change (from |
@nknize I have difficulties understanding the reason for this change
What exactly is stopping other libraries to use
We have more or less concise set of modules, why do we want to beef everything into |
On the surface it seems that way so let me address each of these questions separately.
The build is configured such that library modules may not depend on other library modules except
This change isn't beefing everything into core. It's teasing out the reusable generics and interface contracts to the core library which is used to implement the concrete classes and architecture details across modules and concrete libraries.
It seems that way, but if you look at the change it's sticking w/ the intended design and refactoring the XContent base classes and interfaces to the core library while leaving the concrete implementations of those MediaTypes (e.g., The driver for this is to further modularize the AggregationsThird party libraries, plugins, modules, etc, want to build new aggregations all the time. We provide this ability and it's super powerful. To do so is quite boiler plate: 1. If the CoreValuesSourceType library doesn't already provide the requisite ValueSource, write the needed ValuesSourceType the new aggregation depends on, 2. write the Aggregator, 3. write the Aggregator Factory, 4. write the AggregationBuilder, 5. register the aggregation through your plugin using the AggregationSpec As you can see in this process there is a large number of abstract classes and interfaces required to support implementing a new concrete aggregation anywhere in the "ecosystem". Organizationally, these base classes are spread across the aggregations and aggregations.support namespace, along with the mechanisms needed to implement the aggregation phase(s) in the SearchModule and SearchService class implementations. This means for anyone wanting to build a custom aggregation they would take a I think most on this project agree this is a modularization anti-pattern, so to remedy we're striving to make the codebase much more modular. So how do we achieve that here with this clumsy aggregation foundation and user facing "API" (which isn't exactly well documented, or implemented, or supported)? At first pass you might say, "hey, how about we stick with the intent of the libraries and modules design and refactor the generics and base classes from the bloated But if you embark down that journey you will quickly find the cross library dependency restriction blocks much of this objective and the tight dependency requirement of the So that leaves us with a few options:
With this PR I chose Option 4 because it not only fits the modularization objective but after the aggregation framework is refactored to a new module (and supporting library) users in the aggregation scenario above would only need to take a dependency on the opensearch-core and a (soon coming) opensearch-aggregations library to implement a new concrete aggregation in their own plugin. I hope this helps paint a clearer picture? |
This change intends to target 3.0 only (I just added the labels, sorry about that). However, that wouldn't preclude a backport if we decided to pull it into 2.x since mixed cluster rolling upgrades are only mixed nodes not mixed jars. The side effect, however, is a namespace ripple across all plugins that depend on xcontent base classes. I'd wager that's at least 90% of plugins. The refactor would have to be done before releasing the 2.x version that would include this change. |
…earch-project#5902) Refactors the xcontent base classes from the xcontent module to the core library so serializable xcontent contracts can be used across libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> Signed-off-by: Matthew Wells <matthew.wells@improving.com>
…earch-project#5902) Refactors the xcontent base classes from the xcontent module to the core library so serializable xcontent contracts can be used across libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
…earch-project#5902) Refactors the xcontent base classes from the xcontent module to the core library so serializable xcontent contracts can be used across libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
…earch-project#5902) Refactors the xcontent base classes from the xcontent module to the core library so serializable xcontent contracts can be used across libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
…earch-project#5902) Refactors the xcontent base classes from the xcontent module to the core library so serializable xcontent contracts can be used across libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
#6470) Refactors the xcontent base classes from the xcontent module to the core library so serializable xcontent contracts can be used across libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> (cherry picked from commit cd95bc5)
Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. --------- Signed-off-by: MaxKsyunz <maxk@bitquilltech.com> (cherry picked from commit cd95bc5)
* Address build failures (#1366) Xcontent classes were moved in this PR opensearch-project/OpenSearch#5902 causing builds to fail. Signed-off-by: MaxKsyunz <maxk@bitquilltech.com>
This is a breaking change in version 2.10! Where is the communication of breaking changes? This should be part of the changelog. |
@AndreVirtimo you are absolutely right, the change was slipped away but it should have been in the change log and release notes for 2.10, sincere apologies for that. |
@AndreVirtimo Can you share your use case? Where / how did you run into this? Did you get a NoClassDefFound? Do you have an implementation that relies on it? Are you importing dependencies yourself? The reason I ask is because the core technically does not (yet) provide a formal or comprehensive plugin / library API (we're working on this; feel free to track history through #5910) and dependency libraries are (should) already be handled by the min-distribution, bundle, or docker releases. I want to make sure you didn't stumble on a release artifact bug. |
@nknize A breaking change info for developers would be important (+ necessary migration steps). E.g. for a OpenSearch plugin, you must adjust the plugin version for the target OpenSearch version. This means also you must re-compile the plugin. This will break due to this and the other related refactoring. In our plugin, nearly every class (> 200) had to be adjusted. Not only moved classes but also API changes happened in 2.10. (Strings.toString(XContentBuilder) removed etc.) But also in other code that uses the REST High Level Client we use the XContentBuilder quite often and like it. The RestStatus class and others also moved. It was an extremely suprising and big change for a minor release (developer point of view). ;-) |
@olfuerniss thank you for sharing your experiences. It's always helpful to get feedback from others in how they're using the artifacts. It sounds like you're developing your own plugin? Similar to your comments, the OpenSearch plugins in general deal with these kinds of issues every time-based minor release cycle for reasons outlined in #5910. As I mentioned, the refactoring effort is a move to fix this in the future to support an official Developer API and (since it's not there yet and requires all core maintainers pulling in the same direction) will require some rocky moments like this since the project attempts to minimize major releases. Just to add a few things to open the aperture here:
There is a
+1. Until the core API is defined (through JPMS, library exports, and other java mechanisms like |
Refactors the xcontent base classes from the xcontent module to the core library so serializable xcontent contracts can be used across other libraries (e.g., new aggregation library, other community contributed libraries). Since
opensearch-core
is intended to be the foundation library for all of opensearch, this PR continues the effort to refactor those foundation classes from the:server
module to appropriate library and base module plugins in order to reduce the surface area of the:server
module.Note: The surface area of this change is large due to xcontent being used as a foundation across the entire project.
relates #5910
depends on #6218