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] Various Utilities, Assertion, and Concurrency Exception from server to libraries #6875

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Mar 29, 2023

To further reduce the surface area of utility methods in :server: this commit refactors the following:

  • MapBuilder from server module to common library
  • Assertions from server module to core library
  • BytesRef methods from CollectionUtils in server module to new BytesRefUtils in core library

It also removes CollectionUtils dependency on hppc in prep for refactoring CollectionUtils methods to the proper library.

related #5910

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.snapshots.SearchableSnapshotIT.testCacheFilesAreClosedAfterUse
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.split/30_copy_settings/Copy settings during split index}
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.shrink/30_copy_settings/Copy settings during shrink index}

@codecov-commenter
Copy link

codecov-commenter commented Mar 29, 2023

Codecov Report

Merging #6875 (5881606) into main (7b708c4) will increase coverage by 0.17%.
The diff coverage is 84.21%.

📣 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    #6875      +/-   ##
============================================
+ Coverage     70.52%   70.70%   +0.17%     
- Complexity    59112    59235     +123     
============================================
  Files          4814     4812       -2     
  Lines        283743   283717      -26     
  Branches      40915    40915              
============================================
+ Hits         200110   200594     +484     
+ Misses        67154    66615     -539     
- Partials      16479    16508      +29     
Impacted Files Coverage Δ
...java/org/opensearch/common/collect/MapBuilder.java 66.66% <ø> (ø)
.../src/main/java/org/opensearch/core/Assertions.java 75.00% <ø> (ø)
...currency/OpenSearchRejectedExecutionException.java 100.00% <ø> (ø)
...ch/index/reindex/remote/RemoteResponseParsers.java 90.75% <ø> (ø)
...src/main/java/org/opensearch/ExceptionsHelper.java 79.14% <ø> (-3.07%) ⬇️
...rg/opensearch/action/bulk/TransportBulkAction.java 77.85% <ø> (ø)
...org/opensearch/action/support/RetryableAction.java 85.89% <ø> (ø)
...tion/support/replication/ReplicationOperation.java 77.63% <ø> (+3.10%) ⬆️
...upport/replication/TransportReplicationAction.java 77.45% <ø> (+1.07%) ⬆️
...opensearch/cluster/InternalClusterInfoService.java 75.43% <ø> (-0.87%) ⬇️
... and 46 more

... and 495 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.

@nknize nknize added v3.0.0 Issues and PRs related to version 3.0.0 >breaking Identifies a breaking change. skip-changelog and removed >breaking Identifies a breaking change. labels Mar 29, 2023
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.backwards.MixedClusterClientYamlTestSuiteIT.test {p0=indices.split/30_copy_settings/Copy settings during split index}

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented Mar 30, 2023

oye.. bouncycastle FIPS WhiteSource failure bonanza continues... :/

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@@ -79,6 +79,9 @@ dependencies {

api "com.fasterxml.jackson.core:jackson-core:${versions.jackson}"

// lucene
api "org.apache.lucene:lucene-core:${versions.lucene}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nknize I have doubts we need to depend on Lucene in any core / common modules, the BytesRefUtils is very specialized utility class, why do we need to promote it to core?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per #5910. Consider a plugin that has it's own custom Analyzer using AnalysisPlugin. Downstream plugins implement a TokenFilterFactory which needs Lucene's TokenStream. We need to take a dependency on lucene for this in whatever library houses those contracts (currently decided to be the opensearch-core library).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Separately from that, I could remove BytesRefUtils altogether as that logic isn't actually used anywhere. I chose to keep it (for now) as a convenience function for external plugins.

Copy link
Collaborator

@reta reta Mar 30, 2023

Choose a reason for hiding this comment

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

We need to take a dependency on lucene for this in whatever library houses those contracts (currently decided to be the opensearch-core library).

This is totally fine, and I believe by importing AnalysisPlugin the Lucene will be brought in, but the core does not need it and should not bring it I think. Take libs for example - they hardly ever need to know about Lucene but very likely will depend on core / common.

Separately from that, I could remove BytesRefUtils altogether as that logic isn't actually used anywhere.

👍 ty

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

...but the core does not need it and should not bring it I think

I think we're not on the same page. #5910 plans to move general contracts like AnalysisPlugin (and all of the others) out of :server and into opensearch-core library so plugins only need take a dependency on the API not the entire :server module. To do that requires classes like *Plugin, be refactored out of :server and into libs:opensearch-core. If you refactor AnalysisPlugin to :opensearch-core you need :opensearch-core to take a dependency on lucene. So, the core library will need this lucene dependency, otherwise these contracts have to stay in :server

Copy link
Collaborator

@reta reta Mar 31, 2023

Choose a reason for hiding this comment

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

What's the different route?

First thing, remove dependency to opensearch-core where opensearch-common would be sufficient (possibly moving few classes between modules). With that - we could be 1000% sure that no new transitive dependencies would surprise us along the away.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In the meantime I updated this PR to remove opensearch-core dependency from ssl-config and nio libraries.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@nknize libs/opensearch-cli ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 done

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was hoping all of this would be done incrementally after this PR was merged.

There is an argument for moving the xcontent classes out of core into the common library now and letting core take the dependency on Lucene and other third-party libraries. Which would mean all of the plugins would revert the PRs they just merged. This is why we need to decouple downstream repos to enable us to move on core changes.

…om server to libraries

To further reduce the surface area of utility methods in :server: this commit refactors the
following:

 * MapBuilder from server module to common library
 * Assertions from server module to core library
 * BytesRef methods from CollectionUtils in server module to
   new BytesRefUtils in core library

It also removes CollectionUtils dependency on hppc in prep for
refactoring CollectionUtils methods to the proper library.

Signed-off-by: Nicholas Walter Knize <[email protected]>
@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:

@nknize nknize merged commit 0a20092 into opensearch-project:main Apr 3, 2023
@nknize nknize added the backport 2.x Backport to 2.x branch label Apr 3, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

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-6875-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 0a2009250cd857d68cdfa87340f8cd1b906d8c6e
# Push it to GitHub
git push --set-upstream origin backport/backport-6875-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-6875-to-2.x.

mitrofmep pushed a commit to mitrofmep/OpenSearch that referenced this pull request Apr 5, 2023
…om server to libraries (opensearch-project#6875)

To further reduce the surface area of utility methods in :server: this commit refactors the
following:

 * MapBuilder from server module to common library
 * Assertions from server module to core library
 * BytesRef methods from CollectionUtils in server module to
   new BytesRefUtils in core library

It also removes CollectionUtils dependency on hppc in prep for
refactoring CollectionUtils methods to the proper library.

Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Valentin Mitrofanov <[email protected]>
@ashking94
Copy link
Member

@nknize can we backport this PR to 2.x? Seeing failure in backport #7364 of original PR #7227.

linuxpi pushed a commit to linuxpi/OpenSearch that referenced this pull request May 17, 2023
…om server to libraries (opensearch-project#6875)

To further reduce the surface area of utility methods in :server: this commit refactors the
following:

 * MapBuilder from server module to common library
 * Assertions from server module to core library
 * BytesRef methods from CollectionUtils in server module to
   new BytesRefUtils in core library

It also removes CollectionUtils dependency on hppc in prep for
refactoring CollectionUtils methods to the proper library.

Signed-off-by: Nicholas Walter Knize <[email protected]>
(cherry picked from commit 0a20092)
linuxpi pushed a commit to linuxpi/OpenSearch that referenced this pull request May 19, 2023
…om server to libraries (opensearch-project#6875)

To further reduce the surface area of utility methods in :server: this commit refactors the
following:

 * MapBuilder from server module to common library
 * Assertions from server module to core library
 * BytesRef methods from CollectionUtils in server module to
   new BytesRefUtils in core library

It also removes CollectionUtils dependency on hppc in prep for
refactoring CollectionUtils methods to the proper library.

Signed-off-by: Nicholas Walter Knize <[email protected]>
(cherry picked from commit 0a20092)
gbbafna pushed a commit that referenced this pull request May 19, 2023
…om server to libraries (#6875) (#7612)

To further reduce the surface area of utility methods in :server: this commit refactors the
following:

 * MapBuilder from server module to common library
 * Assertions from server module to core library
 * BytesRef methods from CollectionUtils in server module to
   new BytesRefUtils in core library

It also removes CollectionUtils dependency on hppc in prep for
refactoring CollectionUtils methods to the proper library.

Signed-off-by: Nicholas Walter Knize <[email protected]>
(cherry picked from commit 0a20092)

Signed-off-by: bansvaru <[email protected]>
Co-authored-by: Nicholas Walter Knize  <[email protected]>
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 skip-changelog 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.

4 participants