-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Change abstraction point for transport protocol #15432
Change abstraction point for transport protocol #15432
Conversation
@reta It was easier to illustrate the change here as a PR as opposed to describing it in an issue. The key changes here are basically:
The upshot of the change here is that the "header" part of the transport protocol remains unchanged, but they payload of each message is what now can be different protocols. The server will be able to freely exchange a mix of protocols on one connection at runtime (required for bwc anyway). The following code is where we will switch to the appropriate payload handler based on the protocol specified in the header: OpenSearch/server/src/main/java/org/opensearch/transport/InboundHandler.java Lines 110 to 122 in 0138b6e
|
❌ Gradle check result for 0138b6e: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
This looks right to me! |
Double it! |
0138b6e
to
d4217c1
Compare
server/src/main/java/org/opensearch/transport/InboundAggregator.java
Outdated
Show resolved
Hide resolved
❕ Gradle check result for d4217c1: UNSTABLE
Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #15432 +/- ##
============================================
+ Coverage 71.79% 71.88% +0.09%
- Complexity 63263 63305 +42
============================================
Files 5231 5233 +2
Lines 296526 296526
Branches 42832 42827 -5
============================================
+ Hits 212900 213171 +271
+ Misses 66121 65782 -339
- Partials 17505 17573 +68 ☔ View full report in Codecov by Sentry. |
d4217c1
to
95eeaf0
Compare
❌ Gradle check result for 95eeaf0: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
c3c7a27
to
4c6b24f
Compare
❌ Gradle check result for 4c6b24f: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
4c6b24f
to
27a7a59
Compare
❌ Gradle check result for 27a7a59: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
27a7a59
to
8580ace
Compare
server/src/main/java/org/opensearch/transport/InboundBytesHandler.java
Outdated
Show resolved
Hide resolved
❕ Gradle check result for 8580ace: UNSTABLE Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure. |
…ation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com>
The previous implementation had a transport switch point in InboundPipeline when the bytes were initially pulled off the wire. There was no implementation for any other protocol as the `canHandleBytes` method was hardcoded to return true. I believe this is the wrong point to switch on the protocol. This change makes NativeInboundBytesHandler protocol agnostic beyond the header. With this change, a complete message is parsed from the stream of bytes, with the header schema being unchanged from what exists today. The protocol switch point will now be at `InboundHandler::inboundMessage`. The header will indicate what protocol was used to serialize the the non-header bytes of the message and then invoke the appropriate handler based on that field. Signed-off-by: Andrew Ross <andrross@amazon.com>
8580ace
to
f6d5ce9
Compare
❌ Gradle check result for f6d5ce9: FAILURE Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change? |
The backport to
To backport manually, run these commands in your terminal: # Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-15432-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 5663b4ae5c5553b358226f15f17b89aa843f9479
# Push it to GitHub
git push --set-upstream origin backport/backport-15432-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x Then, create a pull request where the |
@andrross backport manually pls? |
…5432) * Revert "Replacing InboundMessage with NativeInboundMessage for deprecation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com> * Change abstraction point for transport protocol The previous implementation had a transport switch point in InboundPipeline when the bytes were initially pulled off the wire. There was no implementation for any other protocol as the `canHandleBytes` method was hardcoded to return true. I believe this is the wrong point to switch on the protocol. This change makes NativeInboundBytesHandler protocol agnostic beyond the header. With this change, a complete message is parsed from the stream of bytes, with the header schema being unchanged from what exists today. The protocol switch point will now be at `InboundHandler::inboundMessage`. The header will indicate what protocol was used to serialize the the non-header bytes of the message and then invoke the appropriate handler based on that field. Signed-off-by: Andrew Ross <andrross@amazon.com> --------- Signed-off-by: Andrew Ross <andrross@amazon.com>
…5432) * Revert "Replacing InboundMessage with NativeInboundMessage for deprecation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com> * Change abstraction point for transport protocol The previous implementation had a transport switch point in InboundPipeline when the bytes were initially pulled off the wire. There was no implementation for any other protocol as the `canHandleBytes` method was hardcoded to return true. I believe this is the wrong point to switch on the protocol. This change makes NativeInboundBytesHandler protocol agnostic beyond the header. With this change, a complete message is parsed from the stream of bytes, with the header schema being unchanged from what exists today. The protocol switch point will now be at `InboundHandler::inboundMessage`. The header will indicate what protocol was used to serialize the the non-header bytes of the message and then invoke the appropriate handler based on that field. Signed-off-by: Andrew Ross <andrross@amazon.com> --------- Signed-off-by: Andrew Ross <andrross@amazon.com>
…5432) * Revert "Replacing InboundMessage with NativeInboundMessage for deprecation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com> * Change abstraction point for transport protocol The previous implementation had a transport switch point in InboundPipeline when the bytes were initially pulled off the wire. There was no implementation for any other protocol as the `canHandleBytes` method was hardcoded to return true. I believe this is the wrong point to switch on the protocol. This change makes NativeInboundBytesHandler protocol agnostic beyond the header. With this change, a complete message is parsed from the stream of bytes, with the header schema being unchanged from what exists today. The protocol switch point will now be at `InboundHandler::inboundMessage`. The header will indicate what protocol was used to serialize the the non-header bytes of the message and then invoke the appropriate handler based on that field. Signed-off-by: Andrew Ross <andrross@amazon.com> --------- Signed-off-by: Andrew Ross <andrross@amazon.com>
…5432) * Revert "Replacing InboundMessage with NativeInboundMessage for deprecation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com> * Change abstraction point for transport protocol The previous implementation had a transport switch point in InboundPipeline when the bytes were initially pulled off the wire. There was no implementation for any other protocol as the `canHandleBytes` method was hardcoded to return true. I believe this is the wrong point to switch on the protocol. This change makes NativeInboundBytesHandler protocol agnostic beyond the header. With this change, a complete message is parsed from the stream of bytes, with the header schema being unchanged from what exists today. The protocol switch point will now be at `InboundHandler::inboundMessage`. The header will indicate what protocol was used to serialize the the non-header bytes of the message and then invoke the appropriate handler based on that field. Signed-off-by: Andrew Ross <andrross@amazon.com> --------- Signed-off-by: Andrew Ross <andrross@amazon.com>
The previous implementation had a transport switch point in InboundPipeline when the bytes were initially pulled off the wire. There was no implementation for any other protocol as the
canHandleBytes
method was hardcoded to return true. I believe this is the wrong point to switch on the protocol. This change makes NativeInboundBytesHandler protocol agnostic beyond the header. With this change, a complete message is parsed from the stream of bytes, with the header schema being unchanged from what exists today. The protocol switch point will now be atInboundHandler::inboundMessage
. The header will indicate what protocol was used to serialize the the non-header bytes of the message and then invoke the appropriate handler based on that field.Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.