Skip to content

Conversation

@hasnain-db
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds helper classes for SSL RPC communication that are needed to work around the fact that netty does not support zero-copy transfers.

These mirror the existing MessageWithHeader and MessageEncoder classes with very minor differences. But the differences were just enough that it didn't seem easy to refactor/consolidate, and since we don't expect these classes to change much I hope it's ok.

Why are the changes needed?

These are needed to support transferring ManagedBuffers into a form that can be transferred by netty over the network, since netty's encryption support does not support zero-copy transfers.

Does this PR introduce any user-facing change?

No

How was this patch tested?

Added unit tests

build/sbt
> project network-common
> testOnly org.apache.spark.network.protocol.EncryptedMessageWithHeaderSuite

The rest of the changes and integration were tested as part of #42685

Was this patch authored or co-authored using generative AI tooling?

No

@github-actions github-actions bot added the CORE label Oct 6, 2023
Copy link
Contributor

@mridulm mridulm left a comment

Choose a reason for hiding this comment

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

While waiting for tests, please fix the nit as well.
It wont impact the test result anyway

@hasnain-db
Copy link
Contributor Author

org.apache.spark.sql.kafka010.KafkaSourceStressSuite is failing on this and the other outstanding PR, and seems to be unrelated to the changes here. I wonder if that is related to the new kafka version bump since it looks like it also failed on https://github.com/apache/spark/actions/runs/6497942586/job/17648192318

@mridulm
Copy link
Contributor

mridulm commented Oct 14, 2023

Can you retrigger the tests please ? There are a bunch of failures.

@hasnain-db
Copy link
Contributor Author

done, will re-request review once failures are minimized

@hasnain-db
Copy link
Contributor Author

@mridulm I believe the only failure now is org.apache.spark.sql.kafka010.KafkaSourceStressSuite

@hasnain-db hasnain-db requested a review from mridulm October 15, 2023 04:37
@mridulm
Copy link
Contributor

mridulm commented Oct 15, 2023

There was a failure in KafkaSourceStressSuite, which is unrelated to this PR.
Merged to master.

Thanks for working on this @hasnain-db !

@mridulm mridulm closed this in 41420ca Oct 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants