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

[Refactor] core.common to new opensearch-common library #5976

Merged
merged 8 commits into from
Jan 31, 2023

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Jan 23, 2023

Refactors all of o.opensearch.common classes in the core library to a new opensearch-commons library (inspired by apache commons). The intent is to refactor any o.opensearch.common.* classes in :server module to this new library. This will be done with care such that the dependencies are carefully managed to avoid any cyclic or unnecessarily complicated dependencies.

relates #5910

@nknize nknize added enhancement Enhancement or improvement to existing feature or request Clients Clients within the Core repository such as High level Rest client and low level client v3.0.0 Issues and PRs related to version 3.0.0 labels Jan 23, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@reta
Copy link
Collaborator

reta commented Jan 23, 2023

@nknize I am a bit lost now, I thought that #5902 concluded we don't want/need to have opensearch-commons, the opensearch-core should the one? Also, those 2 always go together, this is what every build.gradle now has:

  api project(':libs:opensearch-commons')
  api project(':libs:opensearch-core')

Is the opensearch-commons is useful by itself?

On a slightly different subject, it seems like the convention for other modules in OpenSearch is to use common (not commons) suffix: analysis-common, ingest-common, ...

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov-commenter
Copy link

codecov-commenter commented Jan 28, 2023

Codecov Report

Merging #5976 (8e15b58) into main (78d0d96) will increase coverage by 0.00%.
The diff coverage is 0.00%.

📣 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    #5976   +/-   ##
=========================================
  Coverage     70.89%   70.90%           
+ Complexity    58839    58816   -23     
=========================================
  Files          4775     4775           
  Lines        280993   280993           
  Branches      40590    40590           
=========================================
+ Hits         199224   199230    +6     
+ Misses        65436    65383   -53     
- Partials      16333    16380   +47     
Impacted Files Coverage Δ
...earch/gradle/precommit/JarHellPrecommitPlugin.java 31.25% <0.00%> (ø)
...rc/main/java/org/opensearch/bootstrap/JarHell.java 77.00% <ø> (ø)
...java/org/opensearch/bootstrap/JdkJarHellCheck.java 0.00% <ø> (ø)
.../src/main/java/org/opensearch/common/Booleans.java 59.32% <ø> (ø)
...rc/main/java/org/opensearch/common/CharArrays.java 56.00% <ø> (ø)
...mmon/src/main/java/org/opensearch/common/Glob.java 37.50% <ø> (ø)
...n/java/org/opensearch/common/MemoizedSupplier.java 100.00% <ø> (ø)
...n/src/main/java/org/opensearch/common/SetOnce.java 100.00% <ø> (ø)
.../main/java/org/opensearch/common/collect/List.java 83.33% <ø> (ø)
...c/main/java/org/opensearch/common/collect/Map.java 93.33% <ø> (ø)
... and 477 more

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

/**
* Java 9 Map
*
* todo: deprecate and remove w/ min jdk upgrade to 11?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we do that since 3.0.0 has JDK-11 baseline?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was planning it in a follow up PR.

@nknize nknize removed the WIP Work in progress label Jan 28, 2023
@reta
Copy link
Collaborator

reta commented Jan 28, 2023

I think LGTM, @andrross what do you think? A side note, @nknize could you please update #5910, I believe the presence of opensearch-common is more or less justified, but not formalized, thank you.

@nknize
Copy link
Collaborator Author

nknize commented Jan 30, 2023

...could you please update #5910

Done.

CHANGELOG.md Outdated Show resolved Hide resolved
Refactors all of o.opensearch.common classes in the core library to a new
opensearch-commons library (inspired by apache commons). The intent is to
refactor any o.opensearch.common.* classes in :server module to this new
library. This will be done with care such that the dependencies are carefully
managed to avoid any cyclic or unnecessarily complicated dependencies.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

*
* todo: deprecate and remove w/ min jdk upgrade to 11?
*
* @opensearch.internal
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nitpick, but I think this is a bit of an abuse of the @opensearch.internal annotation. This was clearly meant to be a public utility just like anything else in the common namespace. It just has been superseded by an alternative, which is exactly what the java.lang.Deprecated annotation is meant to convey.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize nknize merged commit c557f27 into opensearch-project:main Jan 31, 2023
@nknize nknize added the backport 2.x Backport to 2.x branch label Jan 31, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jan 31, 2023
Refactors all of o.opensearch.common classes in the core library to a new
opensearch-commons library (inspired by apache commons). The intent is to
refactor any o.opensearch.common.* classes in :server module to this new
library. This will be done with care such that the dependencies are carefully
managed to avoid any cyclic or unnecessarily complicated dependencies.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
(cherry picked from commit c557f27)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch Clients Clients within the Core repository such as High level Rest client and low level client enhancement Enhancement or improvement to existing feature or request v3.0.0 Issues and PRs related to version 3.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants