Skip to content

Conversation

@hasnain-db
Copy link
Contributor

What changes were proposed in this pull request?

As the title suggests. In addition to that API, add a config to the TransportConf to configure the default block size if desired.

Why are the changes needed?

Netty's SSL support does not support zero-copy transfers. In order to support SSL using Netty we need to add another API to the ManagedBuffer which lets buffers return a different data type.

Does this PR introduce any user-facing change?

No

How was this patch tested?

CI. This will have tests added later - it's tested as part of #42685 from which this is split out.

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

No

@github-actions github-actions bot added the CORE label Sep 28, 2023
@hasnain-db hasnain-db changed the title [SPARK-44937][CORE] Add convertToNettyForSsl to ManagedBuffer [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer Sep 28, 2023
@hasnain-db
Copy link
Contributor Author

cc: @mridulm @JoshRosen this is now ready to review and has all green tests on CI

@mridulm
Copy link
Contributor

mridulm commented Oct 1, 2023

QQ: If/when there is more widespread adoption of kernel.ssl.sendfile (and ssl_sendfile from openssl), how will it impact the proposed design ?

@hasnain-db
Copy link
Contributor Author

@mridulm once it's widely adopted and supported in Netty + netty-tcnative, we can remove these APIs and potentially just use convertToNetty, depending on how the design is from the netty side. But it's unclear to me what the timelines are for that

@mridulm
Copy link
Contributor

mridulm commented Oct 1, 2023

Sounds good, can you add a comment to that effect in the Trait ?
Rest looks good to me

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.

Pending CI
+CC @JoshRosen

@hasnain-db
Copy link
Contributor Author

CI is also finally green here

@mridulm mridulm closed this in b01dce2 Oct 3, 2023
@mridulm
Copy link
Contributor

mridulm commented Oct 3, 2023

Merged to master.
Thanks for working on this @hasnain-db !
Thanks for review @JoshRosen :-)

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.

3 participants