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] Stream Reader and Write Generics #7465

Merged

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented May 8, 2023

StreamInput and StreamOutput provide the core functionality for marshalling / unmarshalling objects over the transport wire. The class is intended to be generic but it is tightly coupled to the types defined in the server module. Because of this tight coupling, the classes cannot be refactored to the core library, thus all new types are required to be hard coded in the server module.

To decouple this logic and make it more generic across opensearch modules and plugins, this commit introduces a reader and writer registry in a new BaseWriteable interface. The StreamInput and StreamOutput also now inherits from new BaseStreamInput and BaseStreamOutput classes, respectively, located in the core library. This will be the new home for streaming primitives in a follow up commit.

relates #5910

@nknize nknize added enhancement Enhancement or improvement to existing feature or request v2.8.0 'Issues and PRs related to version v2.8.0' labels May 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented May 8, 2023

@saratvemulapalli, Foundation refactor to decouple GeoPoint and Joda so StreamInput and StreamOutput can be refactored to core library. I'll do the same for OpenSearchException in a follow-up after pulling up the primitive write / read methods to the BaseStreamInput BaseStreamOutput classes

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

What's the downstream plugins impact? And are you going to do all the types similar to geo point? Should we do a single pass merge on that to minimize that?

libs/core/build.gradle Outdated Show resolved Hide resolved
@nknize
Copy link
Collaborator Author

nknize commented May 9, 2023

What's the downstream plugins impact?

BWC tests cover the downstream impact to streamable. I can't speak, however, to how every other downstream plugin overrides internal APIs. Just like ImmutableOpenMap, they'll find out what needs to be changed when the plugin is built off the new nightly.

are you going to do all the types similar to geo point.

No. We inherited the read/writeGeneric from elastic and it is a terrible design that needs to die. We just have to carry it for BWC. I'll be deprecating this in a follow-up. I'll also (as the description states) be moving the primitives to the new base classes.

Should we do a single pass merge on that to minimize that?

I'll be doing the primitives in a single pass PR, yes.

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.indices.replication.SegmentReplicationAllocationIT.testSingleIndexShardAllocation

@codecov-commenter
Copy link

codecov-commenter commented May 10, 2023

Codecov Report

Merging #7465 (f71b607) into main (20ad1d8) will increase coverage by 0.12%.
The diff coverage is 90.62%.

@@             Coverage Diff              @@
##               main    #7465      +/-   ##
============================================
+ Coverage     70.47%   70.60%   +0.12%     
- Complexity    59692    59774      +82     
============================================
  Files          4893     4896       +3     
  Lines        286748   286779      +31     
  Branches      41327    41331       +4     
============================================
+ Hits         202091   202485     +394     
+ Misses        68060    67576     -484     
- Partials      16597    16718     +121     
Impacted Files Coverage Δ
...earch/aggregations/bucket/geogrid/BaseGeoGrid.java 95.08% <ø> (-1.64%) ⬇️
...earch/aggregations/bucket/geogrid/GeoHashGrid.java 90.00% <ø> (ø)
...earch/aggregations/bucket/geogrid/GeoTileGrid.java 90.00% <ø> (ø)
...pensearch/core/common/io/stream/BaseWriteable.java 61.53% <61.53%> (ø)
...opensearch/script/JodaCompatibleZonedDateTime.java 45.26% <91.66%> (+10.32%) ⬆️
...ibs/core/src/main/java/org/opensearch/Version.java 77.33% <100.00%> (+0.49%) ⬆️
...nsearch/core/common/io/stream/BaseStreamInput.java 100.00% <100.00%> (ø)
...search/core/common/io/stream/BaseStreamOutput.java 100.00% <100.00%> (ø)
...java/org/opensearch/common/geo/GeoBoundingBox.java 89.34% <100.00%> (ø)
.../main/java/org/opensearch/common/geo/GeoPoint.java 89.18% <100.00%> (+1.43%) ⬆️
... and 6 more

... and 507 files with indirect coverage changes

@nknize nknize requested a review from reta May 11, 2023 18:17
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented May 11, 2023

EC2 connection failure.. refired gradle check

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented May 11, 2023

oye.. caught in a version rip current. Rebased.

nknize added 3 commits May 11, 2023 14:23
StreamInput and StreamOutput provide the core functionality for
marshalling / unmarshalling objects over the transport wire. The class
is intended to be generic but it is tightly coupled to the types defined
in the server module. Because of this tight coupling, the classes cannot
be refactored to the core library, thus all new types are required to be
hard coded in the server module.

To decouple this logic and make it more generic across opensearch
modules and plugins, this commit introduces a reader and writer registry
in a new BaseWriteable interface. The StreamInput and StreamOutput also
now inherits from new BaseStreamInput and BaseStreamOutput classes,
respectively, located in the core library. This will be the new home for
streaming primitives in a follow up commit.

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>
@nknize nknize force-pushed the refactor/StreamReadWriteGenerics branch from b75c882 to f71b607 Compare May 11, 2023 19:23
Copy link
Collaborator

@reta reta left a comment

Choose a reason for hiding this comment

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

Love it :-)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize nknize merged commit 3923257 into opensearch-project:main May 11, 2023
*
* @opensearch.internal
*/
public abstract class BaseStreamOutput extends OutputStream {}
Copy link
Member

Choose a reason for hiding this comment

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

Similar to another PR where we're doing quite a bit of Base*, should this have been called OutputStream extends java.io.OutputStream, or at least BaseOutputStream extends ... to match the order of StreamOutput and OutputStream?

@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-7465-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 392325705eca9afe54d3ed38063b1ee7e0263fa2
# Push it to GitHub
git push --set-upstream origin backport/backport-7465-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-7465-to-2.x.

nknize added a commit to nknize/OpenSearch that referenced this pull request Jul 5, 2023
StreamInput and StreamOutput provide the core functionality for
marshalling / unmarshalling objects over the transport wire. The class
is intended to be generic but it is tightly coupled to the types defined
in the server module. Because of this tight coupling, the classes cannot
be refactored to the core library, thus all new types are required to be
hard coded in the server module.

To decouple this logic and make it more generic across opensearch
modules and plugins, this commit introduces a reader and writer registry
in a new BaseWriteable interface. The StreamInput and StreamOutput also
now inherits from new BaseStreamInput and BaseStreamOutput classes,
respectively, located in the core library. This will be the new home for
streaming primitives in a follow up commit.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
reta pushed a commit that referenced this pull request Jul 5, 2023
…8457)

* [Refactor] Stream Reader and Write Generics (#7465)

StreamInput and StreamOutput provide the core functionality for
marshalling / unmarshalling objects over the transport wire. The class
is intended to be generic but it is tightly coupled to the types defined
in the server module. Because of this tight coupling, the classes cannot
be refactored to the core library, thus all new types are required to be
hard coded in the server module.

To decouple this logic and make it more generic across opensearch
modules and plugins, this commit introduces a reader and writer registry
in a new BaseWriteable interface. The StreamInput and StreamOutput also
now inherits from new BaseStreamInput and BaseStreamOutput classes,
respectively, located in the core library. This will be the new home for
streaming primitives in a follow up commit.

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

* changelog

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>

---------

Signed-off-by: Nicholas Walter Knize <nknize@apache.org>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
StreamInput and StreamOutput provide the core functionality for
marshalling / unmarshalling objects over the transport wire. The class
is intended to be generic but it is tightly coupled to the types defined
in the server module. Because of this tight coupling, the classes cannot
be refactored to the core library, thus all new types are required to be
hard coded in the server module.

To decouple this logic and make it more generic across opensearch
modules and plugins, this commit introduces a reader and writer registry
in a new BaseWriteable interface. The StreamInput and StreamOutput also
now inherits from new BaseStreamInput and BaseStreamOutput classes,
respectively, located in the core library. This will be the new home for
streaming primitives in a follow up commit.

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.

4 participants