[Refactor] Use local opensearch.common.SetOnce instead of lucene's utility class #5947
Conversation
nknize
added
enhancement
nknize
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
owaiskazi19,
adnapibar,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah and
xuezhou25
as code owners
…ility class This copies Apache Lucene's o.a.l.util.SetOnce to o.opensearch.common.SetOnce in the opensearch-core library and refactors all opensearch classes from the lucene implementation to the local opensearch implementation. This accomplishes three objectives: 1. remove the dependency on lucene-core library for classes that only import this one lucene utility class, 2. shield opensearch core dependency on this class from any upstream breaking changes, 3. further decouples other foundation classes from third party libraries so they can be refactored from the server module into opensearch support libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
nknize
force-pushed
the
refactor/setOnce
branch
from
68ed4f1 to
521d6a7
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
|
@nknize I am generally against copying from Apache Lucene (there are so many classes copied already), anyway regarding your point:
Is it realistic to break dependency on |
In general I am as well. But I do have my acceptable boundaries, such as common classes like
There is no dependency on This is to break dependency on classes in the Other positive side effects include shielding OpenSearch from any undesireable changes, bugs, or refactoring that may happen upstream (it is still marked An alternative would be to open an upstream lucene change that teases these common utility classes (and others like |
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #5947 +/- ##
============================================
- Coverage 70.85% 70.84% -0.01%
- Complexity 58711 58794 +83
============================================
Files 4770 4771 +1
Lines 280744 280761 +17
Branches 40539 40540 +1
============================================
- Hits 198911 198897 -14
- Misses 65455 65566 +111
+ Partials 16378 16298 -80
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@nknize I would suggest to add Apache Lucene's SetOnce utility class to forbidden APIs list [1] so when someone accidentally imports it in core, we could catch that and point out for replacement. [1] https://github.com/opensearch-project/OpenSearch/blob/main/buildSrc/src/main/resources/forbidden/ |
That's a great idea. I'll do that here. |
| * This is borrowed from lucene's experimental API. It is not reused to eliminate the dependency | ||
| * on lucene core for such a simple (standalone) utility class that may change beyond OpenSearch needs. | ||
| * | ||
| * @opensearch.internal |
There was a problem hiding this comment.
Aren't these public utilities in the core library fair game to be used by plugins? Several of the plugins packaged in this repo are using this.
There was a problem hiding this comment.
yes! Good call out. I'll change the label to @opensearch.api
There was a problem hiding this comment.
Nothing in libs/core currently has any of these javadoc annotations. Is it fair to say that everything Java public in a library module is part of the API, or do we want to start tagging things with annotations?
There was a problem hiding this comment.
Nothing in libs/core currently has any of these javadoc annotations.
I added them in #5902 (along with enabling missingJavadocs task for lib-core) but started teasing that change into a separate PR.
Is it fair to say that everything Java public in a library module is part of the API
That would be a reasonable assumption but alas, the inherited code did not thoughtfully apply java modifiers. The default behavior of the predecessor was public. I'm not sure what the best course would be for this beyond "proceed at your own risk" at which point maybe start conservatively @opensearch.internal (like my change did) and address on a per class basis? I'm open to majority opinion on this.
There was a problem hiding this comment.
Maybe I'm being naive, but I would think that a library like :core would contain few to none of the cross package dependencies of logically internal pieces like is common in :server, which means we could rely on the Java access qualifiers. If there is anything currently public that should not be, then we could @Deprecate it now and remove/change qualifier in 3.0. Is that tenable?
There was a problem hiding this comment.
I would think that a library like :core would contain few to none of the cross package dependencies of logically internal pieces like is common in :server
Correct. There shouldn't be any cross package dependencies. :server should depend on opensearch-core and no dependency the other way around.
Furthermore, IIRC, the original intent was to have zero dependencies in opensearch-core. I'm not sure what the benefit of that really is (if anything it restricts what can be refactored to core) so #5902 breaks that intent by introducing a dependency on the jackson library (which used to be a dependency in x-content). We could continue to discuss whether we want/need a dependency free commons library in the #5902 PR and if so consider a new dependency free library called opensearch-commons and let opensearch-core bring in dependencies.
| @@ -50,6 +50,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), | |||
| - Migrate client transports to Apache HttpClient / Core 5.x ([#4459](https://github.com/opensearch-project/OpenSearch/pull/4459)) | |||
| - Changed http code on create index API with bad input raising NotXContentException from 500 to 400 ([#4773](https://github.com/opensearch-project/OpenSearch/pull/4773)) | |||
| - Change http code for DecommissioningFailedException from 500 to 400 ([#5283](https://github.com/opensearch-project/OpenSearch/pull/5283)) | |||
| - [Refactor] Use local opensearch.common.SetOnce instead of lucene's utility class ([#5947](https://github.com/opensearch-project/OpenSearch/pull/5947)) | |||
There was a problem hiding this comment.
What's the compatibility concern with backporting this to 2.x? It looks to me like it would be fine since the existing Lucene class remains available for anything that is using it. (If it is possible to backport this change, then the changelog entry should go in the [Unreleased 2.x] section).
There was a problem hiding this comment.
I think there are no compatibility concerns for backport: the dependency graph is not changing so the SetOnce is still available at the same place as before (lucene-core).
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
| @@ -50,6 +50,7 @@ java.nio.channels.SocketChannel#connect(java.net.SocketAddress) | |||
| java.lang.Boolean#getBoolean(java.lang.String) | |||
|
|
|||
| org.apache.lucene.util.IOUtils @ use @org.opensearch.core.internal.io instead | |||
| org.apache.lucene.util.SetOnce @ use @org.opensearch.common.SetOnce instead | |||
Gradle Check (Jenkins) Run Completed with:
|
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-5947-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 963bbe0862f9be5e56617430d1ae38c676855089
# Push it to GitHub
git push --set-upstream origin backport/backport-5947-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.xThen, create a pull request where the |
…ility class (opensearch-project#5947) This copies Apache Lucene's o.a.l.util.SetOnce to o.opensearch.common.SetOnce in the opensearch-core library and refactors all opensearch classes from the lucene implementation to the local opensearch implementation. This accomplishes three objectives: 1. remove the dependency on lucene-core library for classes that only import this one lucene utility class, 2. shield opensearch core dependency on this class from any upstream breaking changes, 3. further decouples other foundation classes from third party libraries so they can be refactored from the server module into opensearch support libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
…ility class (#5947) (#5961) This copies Apache Lucene's o.a.l.util.SetOnce to o.opensearch.common.SetOnce in the opensearch-core library and refactors all opensearch classes from the lucene implementation to the local opensearch implementation. This accomplishes three objectives: 1. remove the dependency on lucene-core library for classes that only import this one lucene utility class, 2. shield opensearch core dependency on this class from any upstream breaking changes, 3. further decouples other foundation classes from third party libraries so they can be refactored from the server module into opensearch support libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
…ility class (#5947) (#5961) This copies Apache Lucene's o.a.l.util.SetOnce to o.opensearch.common.SetOnce in the opensearch-core library and refactors all opensearch classes from the lucene implementation to the local opensearch implementation. This accomplishes three objectives: 1. remove the dependency on lucene-core library for classes that only import this one lucene utility class, 2. shield opensearch core dependency on this class from any upstream breaking changes, 3. further decouples other foundation classes from third party libraries so they can be refactored from the server module into opensearch support libraries. Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
This copies Apache Lucene's SetOnce utility class to
o.opensearch.common.SetOncein theopensearch-corelibrary and refactors all opensearch classes from the lucene implementation to the local opensearch implementation.This accomplishes three objectives:
relates #5910