Adding support for handling multiple transport protocols #12967

VachaShah · 2024-03-28T18:13:00Z

Description

Coming from PR #11910, separating out the transport protocol logic which is part of #11910 into this PR. This PR adds support to make it easier to handle multiple transport protocols.

Related Issues

Part of [META] Protobuf for Search API #10684
Follow up from QueryFetchSearchResult as a proto message and node-to-node communication with protobuf #11910

Check List

New functionality includes testing.
- All tests pass
New functionality has been documented.
- New functionality has javadoc added
Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
Commits are signed per the DCO using --signoff
Commit changes are listed out in CHANGELOG.md file (See: Changelog)
Public documentation issue/PR created

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.

github-actions · 2024-03-28T18:35:30Z

Compatibility status:

Checks if related components are compatible with change 3f08cea

Incompatible components

Skipped components

Compatible components

Compatible components: [https://github.com/opensearch-project/custom-codecs.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git]

dblock

I like the small PR that we can reason about and get merged.

I don't love the existing naming of these classes. Does it make sense to move and rename things now? I would like folders for different implementations like native and proto so there's no confusion that these things are standalone.

For example:

|-transport
    |----InboundMessage [an interface only, `getProtocol()`, no `setProtocol()`] 
    |----native
               |- NativeInboundMessage
    |----proto
               |-ProtoInboundMessage

server/src/main/java/org/opensearch/transport/BaseInboundMessage.java

server/src/main/java/org/opensearch/transport/InboundPipeline.java

github-actions · 2024-03-28T19:02:45Z

✅ Gradle check result for 9d2353a: SUCCESS

VachaShah · 2024-03-28T19:03:34Z

I like the small PR that we can reason about and get merged.

I don't love the existing naming of these classes. Does it make sense to move and rename things now? I would like folders for different implementations like native and proto so there's no confusion that these things are standalone.

For example:
|-transport
    |----InboundMessage [an interface only, `getProtocol()`, no `setProtocol()`] 
    |----native
               |- NativeInboundMessage
    |----proto
               |-ProtoInboundMessage

Renaming would definitely make it clearer, I didn't opt it for existing classes since it would be a breaking change no? For example, InboundMessage is declared as a PublicApi.

codecov · 2024-03-28T19:05:12Z

Codecov Report

Attention: Patch coverage is 88.10976% with 39 lines in your changes are missing coverage. Please review.

Project coverage is 71.39%. Comparing base (b15cb0c) to head (3f08cea).
Report is 139 commits behind head on main.

Files	Patch %	Lines
...org/opensearch/transport/NativeMessageHandler.java	89.32%	11 Missing and 8 partials ⚠️
...port/nativeprotocol/NativeInboundBytesHandler.java	88.73%	0 Missing and 8 partials ⚠️
...java/org/opensearch/transport/InboundPipeline.java	66.66%	1 Missing and 3 partials ⚠️
...transport/nativeprotocol/NativeInboundMessage.java	90.90%	2 Missing and 2 partials ⚠️
.../java/org/opensearch/transport/InboundHandler.java	66.66%	1 Missing and 1 partial ⚠️
...in/java/org/opensearch/transport/TcpTransport.java	0.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12967      +/-   ##
============================================
- Coverage     71.42%   71.39%   -0.03%     
- Complexity    59978    60457     +479     
============================================
  Files          4985     5029      +44     
  Lines        282275   284589    +2314     
  Branches      40946    41221     +275     
============================================
+ Hits         201603   203184    +1581     
- Misses        63999    64619     +620     
- Partials      16673    16786     +113

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

VachaShah · 2024-04-06T00:12:18Z

Made the minor changes in new commit. @reta @msfroh Please take a look. Thank you!

github-actions · 2024-04-06T00:56:45Z

❌ Gradle check result for 8a90d0e: 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?

VachaShah · 2024-04-06T00:57:56Z

❌ Gradle check result for 8a90d0e: 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?

#12197

github-actions · 2024-04-06T01:44:21Z

❌ Gradle check result for 8a90d0e: 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?

server/src/main/java/org/opensearch/transport/TcpTransport.java

server/src/main/java/org/opensearch/transport/InboundMessage.java

github-actions · 2024-04-08T05:25:47Z

❌ Gradle check result for 208d75b: 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?

Signed-off-by: Vacha Shah <[email protected]>

github-actions · 2024-04-08T06:20:00Z

✅ Gradle check result for 3f08cea: SUCCESS

VachaShah · 2024-04-08T16:41:11Z

@dblock Can you please take a look at this again? Thank you!

dblock

This looks straightforward enough for me, if @reta is cool with it lets merge.

server/src/main/java/org/opensearch/transport/InboundHandler.java

opensearch-trigger-bot · 2024-04-08T19:00:51Z

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:

# 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-12967-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d202d90df3991daaf3e20be325cac4b323f0ed23
# Push it to GitHub
git push --set-upstream origin backport/backport-12967-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 base branch is 2.x and the compare/head branch is backport/backport-12967-to-2.x.

VachaShah · 2024-04-08T19:01:53Z

Thank you @dblock and @reta! I will add an issue for the discussion and will backport this manually.

…project#12967) * Adding support for more than one protocol for transport Signed-off-by: Vacha Shah <[email protected]> * Adding CHANGELOG entry Signed-off-by: Vacha Shah <[email protected]> * Addressing comments Signed-off-by: Vacha Shah <[email protected]> * Addressing comments Signed-off-by: Vacha Shah <[email protected]> * Removing determineTransportProtocol Signed-off-by: Vacha Shah <[email protected]> * Determine transport protocol only on first byte reference Signed-off-by: Vacha Shah <[email protected]> * Making InboundBytesHandler closeable Signed-off-by: Vacha Shah <[email protected]> * Fixing close() for InboundPipeline Signed-off-by: Vacha Shah <[email protected]> * Adding DeprecatedAPI annotation to japicmp task Signed-off-by: Vacha Shah <[email protected]> * Fixing for detect breaking changes workflow Signed-off-by: Vacha Shah <[email protected]> * Fixing recursion Signed-off-by: Vacha Shah <[email protected]> --------- Signed-off-by: Vacha Shah <[email protected]>

VachaShah · 2024-04-08T19:20:28Z

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:

# 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-12967-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 d202d90df3991daaf3e20be325cac4b323f0ed23
# Push it to GitHub
git push --set-upstream origin backport/backport-12967-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 base branch is 2.x and the compare/head branch is backport/backport-12967-to-2.x.

Backport PR #13125

…ls (#12967) (#13125) * Adding support for handling multiple transport protocols (#12967) * Adding support for more than one protocol for transport Signed-off-by: Vacha Shah <[email protected]> * Adding CHANGELOG entry Signed-off-by: Vacha Shah <[email protected]> * Addressing comments Signed-off-by: Vacha Shah <[email protected]> * Addressing comments Signed-off-by: Vacha Shah <[email protected]> * Removing determineTransportProtocol Signed-off-by: Vacha Shah <[email protected]> * Determine transport protocol only on first byte reference Signed-off-by: Vacha Shah <[email protected]> * Making InboundBytesHandler closeable Signed-off-by: Vacha Shah <[email protected]> * Fixing close() for InboundPipeline Signed-off-by: Vacha Shah <[email protected]> * Adding DeprecatedAPI annotation to japicmp task Signed-off-by: Vacha Shah <[email protected]> * Fixing for detect breaking changes workflow Signed-off-by: Vacha Shah <[email protected]> * Fixing recursion Signed-off-by: Vacha Shah <[email protected]> --------- Signed-off-by: Vacha Shah <[email protected]> * Fixing precommit for 2.x Signed-off-by: Vacha Shah <[email protected]> --------- Signed-off-by: Vacha Shah <[email protected]>

…project#12967) * Adding support for more than one protocol for transport Signed-off-by: Vacha Shah <[email protected]> * Adding CHANGELOG entry Signed-off-by: Vacha Shah <[email protected]> * Addressing comments Signed-off-by: Vacha Shah <[email protected]> * Addressing comments Signed-off-by: Vacha Shah <[email protected]> * Removing determineTransportProtocol Signed-off-by: Vacha Shah <[email protected]> * Determine transport protocol only on first byte reference Signed-off-by: Vacha Shah <[email protected]> * Making InboundBytesHandler closeable Signed-off-by: Vacha Shah <[email protected]> * Fixing close() for InboundPipeline Signed-off-by: Vacha Shah <[email protected]> * Adding DeprecatedAPI annotation to japicmp task Signed-off-by: Vacha Shah <[email protected]> * Fixing for detect breaking changes workflow Signed-off-by: Vacha Shah <[email protected]> * Fixing recursion Signed-off-by: Vacha Shah <[email protected]> --------- Signed-off-by: Vacha Shah <[email protected]> Signed-off-by: Shivansh Arora <[email protected]>

VachaShah mentioned this pull request Mar 28, 2024

QueryFetchSearchResult as a proto message and node-to-node communication with protobuf #11910

Closed

12 tasks

dblock reviewed Mar 28, 2024

View reviewed changes

VachaShah force-pushed the transport-protocol branch from 90966c8 to 8a90d0e Compare April 6, 2024 00:10

VachaShah requested a review from msfroh April 6, 2024 00:12

reta reviewed Apr 6, 2024

View reviewed changes

server/src/main/java/org/opensearch/transport/TcpTransport.java Outdated Show resolved Hide resolved

reta reviewed Apr 6, 2024

View reviewed changes

server/src/main/java/org/opensearch/transport/InboundMessage.java Show resolved Hide resolved

Fixing recursion

3f08cea

Signed-off-by: Vacha Shah <[email protected]>

VachaShah force-pushed the transport-protocol branch from 208d75b to 3f08cea Compare April 8, 2024 05:29

dblock approved these changes Apr 8, 2024

View reviewed changes

server/src/main/java/org/opensearch/transport/InboundHandler.java Show resolved Hide resolved

dblock added the backport 2.x Backport to 2.x branch label Apr 8, 2024

dblock merged commit d202d90 into opensearch-project:main Apr 8, 2024
36 checks passed

opensearch-trigger-bot bot added the backport-failed label Apr 8, 2024

VachaShah mentioned this pull request Apr 8, 2024

[Backport 2.x] Adding support for handling multiple transport protocols (#12967) #13125

Merged

8 tasks

VachaShah mentioned this pull request Apr 8, 2024

Replacing InboundMessage with NativeInboundMessage for deprecation #13126

Merged

8 tasks

DarshitChanpura mentioned this pull request Apr 9, 2024

Adds saml auth header to differentiate saml requests and prevents auto login as anonymous user when basic authentication fails opensearch-project/security#4152

Merged

2 tasks

wbeckler added the v2.14.0 label Apr 15, 2024

VachaShah mentioned this pull request Apr 18, 2024

Abstracting outbound side of transport #13293

Merged

8 tasks

VachaShah mentioned this pull request May 13, 2024

[META] Protobuf for Search API #10684

Open

4 tasks

BrewTestBot mentioned this pull request May 14, 2024

opensearch 2.14.0 Homebrew/homebrew-core#171722

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding support for handling multiple transport protocols #12967

Adding support for handling multiple transport protocols #12967

VachaShah commented Mar 28, 2024 •

edited by peternied

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

dblock left a comment

github-actions bot commented Mar 28, 2024

VachaShah commented Mar 28, 2024

codecov bot commented Mar 28, 2024 •

edited

Loading

VachaShah commented Apr 6, 2024

github-actions bot commented Apr 6, 2024

VachaShah commented Apr 6, 2024

github-actions bot commented Apr 6, 2024

github-actions bot commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

VachaShah commented Apr 8, 2024

dblock left a comment

opensearch-trigger-bot bot commented Apr 8, 2024

VachaShah commented Apr 8, 2024

VachaShah commented Apr 8, 2024

Adding support for handling multiple transport protocols #12967

Adding support for handling multiple transport protocols #12967

Conversation

VachaShah commented Mar 28, 2024 • edited by peternied Loading

Description

Related Issues

Check List

github-actions bot commented Mar 28, 2024 • edited Loading

Compatibility status:

Incompatible components

Skipped components

Compatible components

dblock left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 28, 2024

VachaShah commented Mar 28, 2024

codecov bot commented Mar 28, 2024 • edited Loading

Codecov Report

VachaShah commented Apr 6, 2024

github-actions bot commented Apr 6, 2024

VachaShah commented Apr 6, 2024

github-actions bot commented Apr 6, 2024

github-actions bot commented Apr 8, 2024

github-actions bot commented Apr 8, 2024

VachaShah commented Apr 8, 2024

dblock left a comment

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Apr 8, 2024

VachaShah commented Apr 8, 2024

VachaShah commented Apr 8, 2024

VachaShah commented Mar 28, 2024 •

edited by peternied

Loading

github-actions bot commented Mar 28, 2024 •

edited

Loading

codecov bot commented Mar 28, 2024 •

edited

Loading