You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "zhtaoxiang (via GitHub)" <gi...@apache.org> on 2024/02/13 00:26:53 UTC
[PR] cache ssl contexts and reuse them [pinot]
zhtaoxiang opened a new pull request, #12404:
URL: https://github.com/apache/pinot/pull/12404
`performance`
Instead of creating a new SslContext for the same TlsConfig multiple times, this PR reuses the same SslContext if it's already created.
1. This saves memory by
a. reducing the number of objects created.
b. only one thread (https://github.com/apache/pinot/blob/master/pinot-common/src/main/java/org/apache/pinot/common/utils/TlsUtils.java#L385) will be created for reloading the same tls store and trust store when tlsconfigs are the same.
2. This can also speed up execution because SslContext only needs to be created ones for the same TlsConfig
Reusing the same SslContext does not change the validation logic and does not introduce security flaws because the same key and ca are used in the old and new implementation.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] cache ssl contexts and reuse them [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12404:
URL: https://github.com/apache/pinot/pull/12404#discussion_r1486994420
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ChannelHandlerFactory.java:
##########
@@ -36,8 +38,9 @@
* The {@code ChannelHandlerFactory} provides all kinds of Netty ChannelHandlers
*/
public class ChannelHandlerFactory {
-
public static final String SSL = "ssl";
+ private static final Map<TlsConfig, SslContext> CLIENT_SSL_CONTEXTS_CACHE = new ConcurrentHashMap<>();
Review Comment:
what about just use hashcode as the key to avoid the object copy?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] cache ssl contexts and reuse them [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12404:
URL: https://github.com/apache/pinot/pull/12404#issuecomment-1940533471
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `33 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`43dadbf`)](https://app.codecov.io/gh/apache/pinot/commit/43dadbfd96a70c19a9ac83bb6c0c35f3fa58bffb?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.73% compared to head [(`4cf5418`)](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 34.87%.
> Report is 1 commits behind head on master.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...che/pinot/core/transport/grpc/GrpcQueryServer.java](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvZ3JwYy9HcnBjUXVlcnlTZXJ2ZXIuamF2YQ==) | 0.00% | [15 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...pache/pinot/common/utils/grpc/GrpcQueryClient.java](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvZ3JwYy9HcnBjUXVlcnlDbGllbnQuamF2YQ==) | 0.00% | [11 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...he/pinot/core/transport/ChannelHandlerFactory.java](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29yZS9zcmMvbWFpbi9qYXZhL29yZy9hcGFjaGUvcGlub3QvY29yZS90cmFuc3BvcnQvQ2hhbm5lbEhhbmRsZXJGYWN0b3J5LmphdmE=) | 25.00% | [6 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...java/org/apache/pinot/common/config/TlsConfig.java](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vY29uZmlnL1Rsc0NvbmZpZy5qYXZh) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #12404 +/- ##
=============================================
- Coverage 61.73% 34.87% -26.87%
+ Complexity 207 6 -201
=============================================
Files 2428 2352 -76
Lines 132828 129074 -3754
Branches 20545 19986 -559
=============================================
- Hits 82007 45011 -36996
- Misses 44811 80843 +36032
+ Partials 6010 3220 -2790
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [custom-integration1](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (ø)` | |
| [integration1](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `0.00% <0.00%> (ø)` | |
| [java-11](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `<0.01% <0.00%> (-61.67%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.86% <8.33%> (-26.76%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.83% <8.33%> (-26.88%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.84% <8.33%> (-26.74%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `34.87% <8.33%> (-26.87%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.74% <8.33%> (-14.99%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `46.74% <8.33%> (-0.18%)` | :arrow_down: |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12404/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/pinot/pull/12404?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] cache ssl contexts and reuse them [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12404:
URL: https://github.com/apache/pinot/pull/12404#discussion_r1487244066
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ChannelHandlerFactory.java:
##########
@@ -61,14 +64,22 @@ public static ChannelHandler getLengthFieldPrepender() {
* The {@code getClientTlsHandler} return a Client side Tls handler that encrypt and decrypt everything.
*/
public static ChannelHandler getClientTlsHandler(TlsConfig tlsConfig, SocketChannel ch) {
- return TlsUtils.buildClientContext(tlsConfig).newHandler(ch.alloc());
+ // Make a copy of the TlsConfig because the TlsConfig is mutable, when the TlsConfig is used as the key of the
+ // CLIENT_SSL_CONTEXTS_CACHE, the TlsConfig should not be changed.
+ TlsConfig tlsConfigCopy = new TlsConfig(tlsConfig);
Review Comment:
The frequency should be low, only at the time when the channel is created. It is the same as how many times the channel should be created.
Anyway, changed the map to use hashCode as the key, so the object copy can be avoided.
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] cache ssl contexts and reuse them [pinot]
Posted by "zhtaoxiang (via GitHub)" <gi...@apache.org>.
zhtaoxiang commented on code in PR #12404:
URL: https://github.com/apache/pinot/pull/12404#discussion_r1487242727
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ChannelHandlerFactory.java:
##########
@@ -36,8 +38,9 @@
* The {@code ChannelHandlerFactory} provides all kinds of Netty ChannelHandlers
*/
public class ChannelHandlerFactory {
-
public static final String SSL = "ssl";
+ private static final Map<TlsConfig, SslContext> CLIENT_SSL_CONTEXTS_CACHE = new ConcurrentHashMap<>();
Review Comment:
That is a great idea!
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] cache ssl contexts and reuse them [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12404:
URL: https://github.com/apache/pinot/pull/12404#discussion_r1486994741
##########
pinot-core/src/main/java/org/apache/pinot/core/transport/ChannelHandlerFactory.java:
##########
@@ -61,14 +64,22 @@ public static ChannelHandler getLengthFieldPrepender() {
* The {@code getClientTlsHandler} return a Client side Tls handler that encrypt and decrypt everything.
*/
public static ChannelHandler getClientTlsHandler(TlsConfig tlsConfig, SocketChannel ch) {
- return TlsUtils.buildClientContext(tlsConfig).newHandler(ch.alloc());
+ // Make a copy of the TlsConfig because the TlsConfig is mutable, when the TlsConfig is used as the key of the
+ // CLIENT_SSL_CONTEXTS_CACHE, the TlsConfig should not be changed.
+ TlsConfig tlsConfigCopy = new TlsConfig(tlsConfig);
Review Comment:
what's the frequency of this call and object copy?
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org
Re: [PR] cache ssl contexts and reuse them [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #12404:
URL: https://github.com/apache/pinot/pull/12404
--
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: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org