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] StreamIO and OpenSearchException foundation to core library #8035

Merged

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented Jun 13, 2023

This PR refactors the Stream IO classes from the :server module to :opensearch-core library to support cloud native and serverless extensions.

The following classes are refactored:

  • StreamInput
  • StreamOutput
  • Writeable
  • NamedWriteable
  • NamedWriteableRegistry
  • Index
  • ShardId
  • BytesReference
  • ByteArray
  • BigArray
  • SecureString
  • Text
  • ParsingException
  • RestStatus

This is a predecessor PR to #7783 such that the namespace is left unchanged. The follow up #7783 PR refactors from the org.opensearch to org.opensearch.core namespace to avoid split package in support of jigsaw modularity. This is done to minimize the change surface area as much as possible.

relates #5910
relates #8110
dependency of #7783

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented Jun 13, 2023

One more commit coming. After quick review of this PR I've decided to eliminate the intermediate BaseOpenSearchException and refactor all of OpenSearchException. I will keep a OpenSearchServerExceptions utility class in the server package for registering the server specific exceptions. This will keep the exception framework more extensible without having to double inherit from OpenSearchException and BaseOpenSearchException

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #8035 (2843bb2) into main (0b775e7) will decrease coverage by 0.13%.
The diff coverage is 88.97%.

@@             Coverage Diff              @@
##               main    #8035      +/-   ##
============================================
- Coverage     70.92%   70.80%   -0.13%     
+ Complexity    56664    56575      -89     
============================================
  Files          4722     4719       -3     
  Lines        267604   267551      -53     
  Branches      39214    39204      -10     
============================================
- Hits         189803   189439     -364     
- Misses        61826    62128     +302     
- Partials      15975    15984       +9     
Impacted Files Coverage Δ
...n/src/main/java/org/opensearch/common/Classes.java 66.66% <ø> (ø)
.../src/main/java/org/opensearch/common/Explicit.java 63.63% <ø> (ø)
...rch/common/ExponentiallyWeightedMovingAverage.java 100.00% <ø> (ø)
...in/java/org/opensearch/common/LocalTimeOffset.java 88.26% <ø> (ø)
...java/org/opensearch/common/MacAddressProvider.java 40.62% <ø> (ø)
...main/java/org/opensearch/common/NamedRegistry.java 100.00% <ø> (ø)
...rg/opensearch/common/RandomBasedUUIDGenerator.java 100.00% <ø> (ø)
...java/org/opensearch/common/SecureRandomHolder.java 50.00% <ø> (ø)
...src/main/java/org/opensearch/common/StopWatch.java 48.33% <ø> (ø)
.../org/opensearch/common/TimeBasedUUIDGenerator.java 84.37% <ø> (ø)
... and 213 more

... and 428 files with indirect coverage changes

@dblock
Copy link
Member

dblock commented Jun 13, 2023

With the changes to hlrs, how badly is it going to mess up opensearch-java?

Copy link
Member

@mch2 mch2 left a comment

Choose a reason for hiding this comment

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

Took a few passes through, lgtm. Given size here should prob get a second set of eyes.

@nknize
Copy link
Collaborator Author

nknize commented Jun 19, 2023

@andrross do you want to give it a quick pass?

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

Gradle Check (Jenkins) Run Completed with:

@nknize nknize requested a review from reta June 19, 2023 17:08
Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Doing a first pass.
Thanks for the changes, this decouples server from steamIO which @VachaShah has been trying to untangle for #6844

@nknize
Copy link
Collaborator Author

nknize commented Jun 19, 2023

...this decouples server from steamIO which @VachaShah has been trying to untangle for #6844

Thanks @saratvemulapalli ... 💯 This makes it a lot easier for ProtoBuf to move to :libs:opensearch-xcontent alongside cbor, smile, yaml, etc and not be forced to land in :server. A great example of decoupling!

Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

Thanks @nknize!!

I am worried all downstream dependencies would break and we should provide enough time for them to consume these changes. Quick search shows 2.2k files[1]

[1] https://github.com/search?q=org%3Aopensearch-project+StreamInput+NOT+repo%3Aopensearch-project%2FOpenSearch+&type=code&p=1

@nknize
Copy link
Collaborator Author

nknize commented Jun 19, 2023

I am worried all downstream dependencies would break...

For this PR, as long as the downstream is taking a dependencies { on opensearch-core the consumer should be happy (with the exception of StreamsUtil.readFully). Regarding the follow up namespace change, we discuss this on #8110

@vmmusings
Copy link
Member

vmmusings commented Jun 20, 2023

@nknize There are failures in mocking Writable objects with NoClassFound Exception as mentioned in above two pull requests. Couldn't figure out the root cause. Any help would be appreciated.

Error details

Caused by: java.lang.NoClassDefFoundError: org/opensearch/core/common/io/stream/BaseStreamOutput
	at java.base/java.lang.Class.getDeclaredMethods0(Native Method)
	at java.base/java.lang.Class.privateGetDeclaredMethods(Class.java:3402)
	at java.base/java.lang.Class.getDeclaredMethods(Class.java:2504)
	at net.bytebuddy.description.method.MethodList$ForLoadedMethods.<init>(MethodList.java:152)
	at net.bytebuddy.description.type.TypeDescription$ForLoadedType.getDeclaredMethods(TypeDescription.java:8940)
	at net.bytebuddy.dynamic.scaffold.InstrumentedType$Factory$Default$1.represent(InstrumentedType.java:438)
	at net.bytebuddy.ByteBuddy.redefine(ByteBuddy.java:886)
	at net.bytebuddy.ByteBuddy.redefine(ByteBuddy.java:861)
	at org.mockito.internal.creation.bytebuddy.InlineBytecodeGenerator.transform(InlineBytecodeGenerator.java:390)
	at java.instrument/java.lang.instrument.ClassFileTransformer.transform(ClassFileTransformer.java:244)
	at java.instrument/sun.instrument.TransformerManager.transform(TransformerManager.java:188)
	at java.instrument/sun.instrument.InstrumentationImpl.transform(InstrumentationImpl.java:541)
	at java.instrument/sun.instrument.InstrumentationImpl.retransformClasses0(Native Method)
	at java.instrument/sun.instrument.InstrumentationImpl.retransformClasses(InstrumentationImpl.java:169)
	at org.mockito.internal.creation.bytebuddy.InlineBytecodeGenerator.triggerRetransformation(InlineBytecodeGenerator.java:281)
	... 113 more
	```

@Yury-Fridlyand Yury-Fridlyand mentioned this pull request Jun 21, 2023
6 tasks
@nknize
Copy link
Collaborator Author

nknize commented Jun 21, 2023

@vamsi-amazon looks like these were resolved due to artifact update lag? LMK

@vmmusings
Copy link
Member

@nknize seems like the issue got fixed itself after artifact update.
Thanks.

imRishN pushed a commit to imRishN/OpenSearch that referenced this pull request Jun 27, 2023
opensearch-project#8035)

This commit refactors the Stream IO classes from the server to core
library to support cloud native and serverless extensions.

The following classes are refactored:

* StreamInput
* StreamOutput
* Writeable
* NamedWriteable
* NamedWriteableRegistry
* Index
* ShardId
* BytesReference
* ByteArray
* BigArray
* SecureString
* Text
* ParsingException
* RestStatus

The namespace is left unchanged but will be refactored in a follow up
commit to avoid split package in order to support jigsaw modularity.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Rishab Nahata <rnnahata@amazon.com>
@nknize nknize added the backport 2.x Backport to 2.x branch label Jul 7, 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-8035-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5a968a8ec59926ce4b696c13473f88e4559f79c9
# Push it to GitHub
git push --set-upstream origin backport/backport-8035-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-8035-to-2.x.

nknize added a commit to nknize/OpenSearch that referenced this pull request Jul 7, 2023
opensearch-project#8035)

This commit refactors the Stream IO classes from the server to core
library to support cloud native and serverless extensions.

The following classes are refactored:

* StreamInput
* StreamOutput
* Writeable
* NamedWriteable
* NamedWriteableRegistry
* Index
* ShardId
* BytesReference
* ByteArray
* BigArray
* SecureString
* Text
* ParsingException
* RestStatus

The namespace is left unchanged but will be refactored in a follow up
commit to avoid split package in order to support jigsaw modularity.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
andrross pushed a commit that referenced this pull request Jul 7, 2023
#8035) (#8540)

This commit refactors the Stream IO classes from the server to core
library to support cloud native and serverless extensions.

The following classes are refactored:

* StreamInput
* StreamOutput
* Writeable
* NamedWriteable
* NamedWriteableRegistry
* Index
* ShardId
* BytesReference
* ByteArray
* BigArray
* SecureString
* Text
* ParsingException
* RestStatus

The namespace is left unchanged but will be refactored in a follow up
commit to avoid split package in order to support jigsaw modularity.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
opensearch-project#8035)

This commit refactors the Stream IO classes from the server to core
library to support cloud native and serverless extensions.

The following classes are refactored:

* StreamInput
* StreamOutput
* Writeable
* NamedWriteable
* NamedWriteableRegistry
* Index
* ShardId
* BytesReference
* ByteArray
* BigArray
* SecureString
* Text
* ParsingException
* RestStatus

The namespace is left unchanged but will be refactored in a follow up
commit to avoid split package in order to support jigsaw modularity.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
Signed-off-by: Shivansh Arora <hishiv@amazon.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 enhancement Enhancement or improvement to existing feature or request skip-changelog v2.9.0 'Issues and PRs related to version v2.9.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants