You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "hasnain-db (via GitHub)" <gi...@apache.org> on 2023/09/28 06:14:10 UTC
[GitHub] [spark] hasnain-db opened a new pull request, #43166: [SPARK-44937][CORE] Add convertToNettyForSsl to ManagedBuffer
hasnain-db opened a new pull request, #43166:
URL: https://github.com/apache/spark/pull/43166
### 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 https://github.com/apache/spark/pull/42685 from which this is split out.
### Was this patch authored or co-authored using generative AI tooling?
No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm closed pull request #43166: [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer
URL: https://github.com/apache/spark/pull/43166
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hasnain-db commented on pull request #43166: [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43166:
URL: https://github.com/apache/spark/pull/43166#issuecomment-1742199158
@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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #43166: [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43166:
URL: https://github.com/apache/spark/pull/43166#issuecomment-1742217688
Sounds good, can you add a comment to that effect in the `Trait` ?
Rest looks good to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] hasnain-db commented on pull request #43166: [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43166:
URL: https://github.com/apache/spark/pull/43166#issuecomment-1740276742
cc: @mridulm @JoshRosen this is now ready to review and has all green tests on CI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer [spark]
Posted by "hasnain-db (via GitHub)" <gi...@apache.org>.
hasnain-db commented on PR #43166:
URL: https://github.com/apache/spark/pull/43166#issuecomment-1744123259
CI is also finally green here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] mridulm commented on pull request #43166: [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43166:
URL: https://github.com/apache/spark/pull/43166#issuecomment-1741949095
QQ: If/when there is more widespread adoption of `kernel.ssl.sendfile` (and `ssl_sendfile` from openssl), how will it impact the proposed design ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-45378][CORE] Add convertToNettyForSsl to ManagedBuffer [spark]
Posted by "mridulm (via GitHub)" <gi...@apache.org>.
mridulm commented on PR #43166:
URL: https://github.com/apache/spark/pull/43166#issuecomment-1744125444
Merged to master.
Thanks for working on this @hasnain-db !
Thanks for review @JoshRosen :-)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org