Replacing InboundMessage with NativeInboundMessage for deprecation #13126

VachaShah · 2024-04-08T21:21:18Z

Description

InboundMessage was deprecated in #12967 and replaced with NativeInboundMessage. Replacing instances in code for the same.

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

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-04-08T21:45:47Z

Compatibility status:

Checks if related components are compatible with change 3d4ab18

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/neural-search.git, https://github.com/opensearch-project/flow-framework.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/performance-analyzer.git]

github-actions · 2024-04-08T21:54:49Z

❌ Gradle check result for 38e668c: 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?

reta · 2024-04-08T21:56:51Z

@VachaShah this is for 3.0 only, right? PS: I think we'll have hard time backporting further changes if we take this path now

VachaShah · 2024-04-08T21:59:44Z

@VachaShah this is for 3.0 only, right? PS: I think we'll have hard time backporting further changes if we take this path now

Yeah this is only for 3.0 since InboundMessage is going to be there in the 2.x line. I did this now so that its not forgotten to be done in the future.

VachaShah · 2024-04-08T22:10:47Z

❌ Gradle check result for 38e668c: 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?

Test Result (1 failure / +1)
org.opensearch.snapshots.InternalSnapshotsInfoServiceTests.testErroneousSnapshotShardSizes

VachaShah · 2024-04-12T22:24:03Z

Or I can remove InboundMessage in this same PR and not backport this PR all together. Let me know what you guys think.

The issue as I see it is that all usages of InboundMessage are replaced with NativeInboundMessage, this is fine but for anyone relying on InboundMessage (fe TcpTransport::inboundMessage(TcpChannel channel, InboundMessage message)), the methods will never be called.

Makes sense! I will remove InboundMessage in this PR and keep this change only for main.

github-actions · 2024-04-12T23:27:17Z

❌ Gradle check result for 3d4ab18: 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?

github-actions · 2024-04-13T09:22:18Z

❌ Gradle check result for 3d4ab18: 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-15T18:59:08Z

Looks like the BWC tests started failing after removing the InboundMessage class all together. Will look into the test failures.

Signed-off-by: Vacha Shah <vachshah@amazon.com>

github-actions · 2024-04-17T19:46:08Z

❌ Gradle check result for adccc66: 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-17T19:58:34Z

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

Test Result (1 failure / +1)
org.opensearch.index.remote.RemoteSegmentTransferTrackerTests.testComputeTimeLagOnUpdate

#12639

github-actions · 2024-04-17T20:48:22Z

✅ Gradle check result for adccc66: SUCCESS

dblock · 2024-04-17T21:35:03Z

@reta Good to merge the breaking change?

reta · 2024-04-17T21:45:58Z

@reta Good to merge the breaking change?

Ah ... @dblock could you please hold on? We've been recently fighting with an issue (along with @peternied ) that merging breaking change in one pull request impacts the checks for other pull requests. There is nothing wrong with breaking change in major release, but we have a flaw in our checks currently.

dblock

Blocking per @reta.

reta · 2024-04-18T14:13:06Z

Blocking per @reta.

The relevant change is under discussion #13283

reta · 2024-04-18T15:02:30Z

@dblock we should be cleared out, the check is disabled on main now

reta · 2024-04-18T16:04:06Z

Note on the failing detect-breaking-change: the pull request was merged with this check failing, the check itself is disabled on main now (#13283)

…ation (opensearch-project#13126)" This reverts commit f5c3ef9.

…ation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com>

* Revert "Replacing InboundMessage with NativeInboundMessage for deprecation (#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>

VachaShah added the skip-changelog label Apr 8, 2024

VachaShah added 2 commits April 17, 2024 19:07

Replacing InboundMessage with NativeInboundMessage for deprecation

07147b8

Signed-off-by: Vacha Shah <vachshah@amazon.com>

Removing InboundMessage class

adccc66

Signed-off-by: Vacha Shah <vachshah@amazon.com>

VachaShah force-pushed the inboundmessage-replace branch from 3d4ab18 to adccc66 Compare April 17, 2024 19:07

dblock approved these changes Apr 17, 2024

View reviewed changes

dblock requested changes Apr 17, 2024

View reviewed changes

reta requested a review from dblock April 18, 2024 15:01

reta approved these changes Apr 18, 2024

View reviewed changes

dblock approved these changes Apr 18, 2024

View reviewed changes

dblock merged commit f5c3ef9 into opensearch-project:main Apr 18, 2024
33 of 34 checks passed

VachaShah mentioned this pull request May 13, 2024

[META] Protobuf for Search API #10684

Open

4 tasks

andrross mentioned this pull request Aug 27, 2024

Change abstraction point for transport protocol #15432

Merged

3 tasks

andrross added a commit to andrross/OpenSearch that referenced this pull request Aug 27, 2024

Revert "Replacing InboundMessage with NativeInboundMessage for deprec…

c49ae89

…ation (opensearch-project#13126)" This reverts commit f5c3ef9.

andrross added a commit to andrross/OpenSearch that referenced this pull request Aug 28, 2024

Revert "Replacing InboundMessage with NativeInboundMessage for deprec…

2a265b7

…ation (opensearch-project#13126)" This reverts commit f5c3ef9.

andrross added a commit to andrross/OpenSearch that referenced this pull request Aug 28, 2024

Revert "Replacing InboundMessage with NativeInboundMessage for deprec…

6a31bcb

…ation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com>

andrross added a commit to andrross/OpenSearch that referenced this pull request Aug 28, 2024

Revert "Replacing InboundMessage with NativeInboundMessage for deprec…

13a2304

…ation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com>

andrross added a commit to andrross/OpenSearch that referenced this pull request Aug 28, 2024

Revert "Replacing InboundMessage with NativeInboundMessage for deprec…

df86871

…ation (opensearch-project#13126)" This reverts commit f5c3ef9. Signed-off-by: Andrew Ross <andrross@amazon.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replacing InboundMessage with NativeInboundMessage for deprecation #13126

Replacing InboundMessage with NativeInboundMessage for deprecation #13126

VachaShah commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024

reta commented Apr 8, 2024

VachaShah commented Apr 8, 2024

VachaShah commented Apr 8, 2024

VachaShah commented Apr 12, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 13, 2024

VachaShah commented Apr 15, 2024

github-actions bot commented Apr 17, 2024

VachaShah commented Apr 17, 2024

github-actions bot commented Apr 17, 2024

dblock commented Apr 17, 2024

reta commented Apr 17, 2024

dblock left a comment

reta commented Apr 18, 2024

reta commented Apr 18, 2024

reta commented Apr 18, 2024

Replacing InboundMessage with NativeInboundMessage for deprecation #13126

Replacing InboundMessage with NativeInboundMessage for deprecation #13126

Conversation

VachaShah commented Apr 8, 2024 • edited Loading

Description

Related Issues

Check List

github-actions bot commented Apr 8, 2024 • edited Loading

Compatibility status:

Incompatible components

Skipped components

Compatible components

github-actions bot commented Apr 8, 2024

reta commented Apr 8, 2024

VachaShah commented Apr 8, 2024

VachaShah commented Apr 8, 2024

VachaShah commented Apr 12, 2024

github-actions bot commented Apr 12, 2024

github-actions bot commented Apr 13, 2024

VachaShah commented Apr 15, 2024

github-actions bot commented Apr 17, 2024

VachaShah commented Apr 17, 2024

github-actions bot commented Apr 17, 2024

dblock commented Apr 17, 2024

reta commented Apr 17, 2024

dblock left a comment

Choose a reason for hiding this comment

reta commented Apr 18, 2024

reta commented Apr 18, 2024

reta commented Apr 18, 2024

VachaShah commented Apr 8, 2024 •

edited

Loading

github-actions bot commented Apr 8, 2024 •

edited

Loading