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] OpenSearchException streamables to a registry #7646

Conversation

nknize
Copy link
Collaborator

@nknize nknize commented May 21, 2023

OpenSearchException uses an enumerator to create a handler for each serializable Exception implementation. The enumerator is to make it easy to store in an immutable map. The problem with this Hack is that it makes it difficult to unwind the tight coupling of StreamInput with the OpenSearchException class. This commit switches from using an enumerator to a registry in the opensearch-core library in order to support serverless and cloud native implementations outside of the server module.

This commit also refactors base primitive serialization readers and writers from StreamInput and StreamOutput to BaseStreamInput and BaseStreamOutput, respectively.

relates #5910

@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 May 22, 2023

Not reproducible, rebasing and refiring

./gradlew ':server:test' --tests "org.opensearch.index.IndexServiceTests.testAsyncTranslogTrimTaskOnClosedIndex" -Dtests.seed=4212AD3BAE883BE9 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ar-BH -Dtests.timezone=CST6CDT -Druntime.java=19

@nknize nknize force-pushed the refactor/OpenSearchExceptionRegistryPrimitiveStreams branch from 57ea42f to f58e8d8 Compare May 22, 2023 15:18
@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented May 22, 2023

another #6287, this was reproducible locally. Looks related to #6143, @RS146BIJAY can you take a look?

./gradlew ':server:internalClusterTest' --tests "org.opensearch.cluster.shards.ClusterShardLimitIT.testCreateIndexWithMaxClusterShardSetting" -Dtests.seed=3094586F159455F2 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=lt-LT -Dtests.timezone=PLT -Druntime.java=19
org.opensearch.cluster.shards.ClusterShardLimitIT > testCreateIndexWithMaxClusterShardSetting FAILED
    java.lang.AssertionError: MaxShardPerCluster 2 should be greater than or equal to MaxShardPerNode 1000
        at __randomizedtesting.SeedInfo.seed([3094586F159455F2:7A72A9D1B8CA934A]:0)
        at org.junit.Assert.fail(Assert.java:89)
        at org.opensearch.cluster.shards.ClusterShardLimitIT.setMaxShardLimit(ClusterShardLimitIT.java:750)
        at org.opensearch.cluster.shards.ClusterShardLimitIT.testCreateIndexWithMaxClusterShardSetting(ClusterShardLimitIT.java:259)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented May 22, 2023

Oye!! Another unrelated failure? Looks like PRs are becoming awfully volatile due to test failures not being regularly fixed.

This one was not reproducible but has already been documented as a "flaky test" and caught on multiple PRs.

./gradlew ':qa:smoke-test-http:integTest' --tests "org.opensearch.http.SearchRestCancellationIT.testAutomaticCancellationMultiSearchDuringQueryPhase" -Dtests.seed=82B60F546F2663DE -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=ru-RU -Dtests.timezone=Asia/Hebron -Druntime.java=19

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@nknize
Copy link
Collaborator Author

nknize commented May 22, 2023

Unreal.. two more (#3590)

Neither are reproducible locally.

./gradlew ':server:internalClusterTest' --tests "org.opensearch.cluster.allocation.ClusterRerouteIT.testDelayWithALargeAmountOfShards" -Dtests.seed=86C4F0E32ABC37F8 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=cs -Dtests.timezone=Atlantic/Jan_Mayen -Druntime.java=19
./gradlew ':server:internalClusterTest' --tests "org.opensearch.search.SearchWeightedRoutingIT.testSearchAggregationWithNetworkDisruption_FailOpenEnabled" -Dtests.seed=86C4F0E32ABC37F8 -Dtests.security.manager=true -Dtests.jvm.argline="-XX:TieredStopAtLevel=1 -XX:ReservedCodeCacheSize=64m" -Dtests.locale=es-CR -Dtests.timezone=Pacific/Pitcairn -Druntime.java=19

@RS146BIJAY
Copy link
Contributor

testDelayWithALargeAmountOfShards

Sure will mute this test.

@nknize
Copy link
Collaborator Author

nknize commented May 22, 2023

testDelayWithALargeAmountOfShards

Sure will mute this test.

That works.. let's try to resolve soon, though so @AwaitsFix doesn't turn into an alias of /dev/null :)

@github-actions
Copy link
Contributor

Gradle Check (Jenkins) Run Completed with:

@codecov
Copy link

codecov bot commented May 22, 2023

Codecov Report

Merging #7646 (2bac9a6) into main (aa21585) will increase coverage by 0.13%.
The diff coverage is 96.88%.

@@             Coverage Diff              @@
##               main    #7646      +/-   ##
============================================
+ Coverage     70.78%   70.92%   +0.13%     
- Complexity    56251    56316      +65     
============================================
  Files          4689     4689              
  Lines        266304   266315      +11     
  Branches      39086    39086              
============================================
+ Hits         188505   188871     +366     
+ Misses        61847    61432     -415     
- Partials      15952    16012      +60     
Impacted Files Coverage Δ
.../org/opensearch/core/common/lease/Releasables.java 78.78% <ø> (ø)
...gregations/matrix/stats/MatrixStatsAggregator.java 75.00% <ø> (-7.50%) ⬇️
...aggregations/bucket/geogrid/GeoGridAggregator.java 81.25% <ø> (ø)
...regations/metrics/AbstractGeoBoundsAggregator.java 56.52% <ø> (ø)
...search/join/aggregations/ParentJoinAggregator.java 73.91% <ø> (ø)
.../transport/netty4/Netty4MessageChannelHandler.java 61.90% <ø> (ø)
...g/opensearch/transport/netty4/Netty4Transport.java 73.65% <ø> (ø)
.../opensearch/transport/nio/TcpReadWriteHandler.java 100.00% <ø> (ø)
...s/close/TransportVerifyShardBeforeCloseAction.java 68.75% <ø> (ø)
...readonly/TransportVerifyShardIndexBlockAction.java 68.29% <ø> (+58.53%) ⬆️
... and 136 more

... and 414 files with indirect coverage changes

nknize added 2 commits June 5, 2023 10:48
OpenSearchException uses an enumerator to create a handler for each
serializable Exception implementation. The enumerator is to make it easy
to store in an immutable map. The problem with this Hack is that it
makes it difficult to unwind the tight coupling of StreamInput with the
OpenSearchException class. This commit switches from using an enumerator
to a registry in the opensearch-core library in order to support
serverless and cloud native implementations outside of the server
module.

This commit also refactors base primitive serialization readers and
writers from StreamInput and StreamOutput to BaseStreamInput and
BaseStreamOutput, respectively.

Signed-off-by: Nicholas Walter Knize <[email protected]>
Signed-off-by: Nicholas Walter Knize <[email protected]>
@nknize nknize force-pushed the refactor/OpenSearchExceptionRegistryPrimitiveStreams branch from f58e8d8 to 2bac9a6 Compare June 5, 2023 15:51
@nknize nknize requested a review from dbwiddis as a code owner June 5, 2023 15:51
@github-actions
Copy link
Contributor

github-actions bot commented Jun 5, 2023

Gradle Check (Jenkins) Run Completed with:

  • RESULT: UNSTABLE ❕
  • TEST FAILURES:
      1 org.opensearch.search.backpressure.SearchBackpressureIT.testSearchShardTaskCancellationWithHighCpu

@dblock dblock merged commit db0933b into opensearch-project:main Jun 6, 2023
@dblock
Copy link
Member

dblock commented Jun 6, 2023

Do you want this in 2.x?

@dblock dblock added backport 2.x Backport to 2.x branch and removed v2.8.0 'Issues and PRs related to version v2.8.0' labels Jun 6, 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-7646-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 db0933b58d10291e5cfe6a95badbe6810d86ffed
# Push it to GitHub
git push --set-upstream origin backport/backport-7646-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-7646-to-2.x.

sandeshkr419 pushed a commit to sandeshkr419/OpenSearch that referenced this pull request Jun 8, 2023
…project#7646)

* [Refactor] OpenSearchException streamables to a registry

OpenSearchException uses an enumerator to create a handler for each
serializable Exception implementation. The enumerator is to make it easy
to store in an immutable map. The problem with this Hack is that it
makes it difficult to unwind the tight coupling of StreamInput with the
OpenSearchException class. This commit switches from using an enumerator
to a registry in the opensearch-core library in order to support
serverless and cloud native implementations outside of the server
module.

This commit also refactors base primitive serialization readers and
writers from StreamInput and StreamOutput to BaseStreamInput and
BaseStreamOutput, respectively.

Signed-off-by: Nicholas Walter Knize <[email protected]>

* revert Project_Default

Signed-off-by: Nicholas Walter Knize <[email protected]>

---------

Signed-off-by: Nicholas Walter Knize <[email protected]>
nknize added a commit to nknize/OpenSearch that referenced this pull request Jul 7, 2023
…project#7646)

OpenSearchException uses an enumerator to create a handler for each
serializable Exception implementation. The enumerator is to make it easy
to store in an immutable map. The problem with this Hack is that it
makes it difficult to unwind the tight coupling of StreamInput with the
OpenSearchException class. This commit switches from using an enumerator
to a registry in the opensearch-core library in order to support
serverless and cloud native implementations outside of the server
module.

This commit also refactors base primitive serialization readers and
writers from StreamInput and StreamOutput to BaseStreamInput and
BaseStreamOutput, respectively.

Signed-off-by: Nicholas Walter Knize <[email protected]>
@nknize nknize added the v2.9.0 'Issues and PRs related to version v2.9.0' label Jul 7, 2023
andrross pushed a commit that referenced this pull request Jul 7, 2023
OpenSearchException uses an enumerator to create a handler for each
serializable Exception implementation. The enumerator is to make it easy
to store in an immutable map. The problem with this Hack is that it
makes it difficult to unwind the tight coupling of StreamInput with the
OpenSearchException class. This commit switches from using an enumerator
to a registry in the opensearch-core library in order to support
serverless and cloud native implementations outside of the server
module.

This commit also refactors base primitive serialization readers and
writers from StreamInput and StreamOutput to BaseStreamInput and
BaseStreamOutput, respectively.

Signed-off-by: Nicholas Walter Knize <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…project#7646)

* [Refactor] OpenSearchException streamables to a registry

OpenSearchException uses an enumerator to create a handler for each
serializable Exception implementation. The enumerator is to make it easy
to store in an immutable map. The problem with this Hack is that it
makes it difficult to unwind the tight coupling of StreamInput with the
OpenSearchException class. This commit switches from using an enumerator
to a registry in the opensearch-core library in order to support
serverless and cloud native implementations outside of the server
module.

This commit also refactors base primitive serialization readers and
writers from StreamInput and StreamOutput to BaseStreamInput and
BaseStreamOutput, respectively.

Signed-off-by: Nicholas Walter Knize <[email protected]>

* revert Project_Default

Signed-off-by: Nicholas Walter Knize <[email protected]>

---------

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

3 participants