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/08 19:37:34 UTC
[PR] add retry to SSLFactory reloading [pinot]
zhtaoxiang opened a new pull request, #12384:
URL: https://github.com/apache/pinot/pull/12384
In our tests, we found that when reloading SSLFactory, need to retry a few times because when one file (key store or trust store) is updated, the other file (trust store or key store) may not have been fully written yet, so we need to wait a bit and retry.
--
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] add retry to SSLFactory reloading [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 commented on code in PR #12384:
URL: https://github.com/apache/pinot/pull/12384#discussion_r1483555919
##########
pinot-common/src/main/java/org/apache/pinot/common/utils/TlsUtils.java:
##########
@@ -447,12 +450,32 @@ static void reloadSslFactoryWhenFileStoreChanges(SSLFactory baseSslFactory,
LOGGER.info("Detected change in file: {}, try to renew SSLFactory {} "
+ "(built from key store {} and truststore {})",
changedFile, baseSslFactory, keyStorePath, trustStorePath);
- SSLFactory updatedSslFactory = createSSLFactory(
- keyStoreType, keyStorePath, keyStorePassword, trustStoreType, trustStorePath, trustStorePassword,
- sslContextProtocol, secureRandom, false);
- SSLFactoryUtils.reload(baseSslFactory, updatedSslFactory);
- LOGGER.info("Successfully renewed SSLFactory {} (built from key store {} and truststore {}) "
- + "on file {} changes", baseSslFactory, keyStorePath, trustStorePath, changedFile);
+ try {
+ // Need to retry a few times because when one file (key store or trust store) is updated, the other file
+ // (trust store or key store) may not have been fully written yet, so we need to wait a bit and retry.
Review Comment:
Please also add a note that it's ok for 3 retries failure, the watcher will be triggered again and renew.
--
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] Fixing the multiple files concurrent write issue when reloading SSLFactory [pinot]
Posted by "xiangfu0 (via GitHub)" <gi...@apache.org>.
xiangfu0 merged PR #12384:
URL: https://github.com/apache/pinot/pull/12384
--
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] add retry to SSLFactory reloading [pinot]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #12384:
URL: https://github.com/apache/pinot/pull/12384#issuecomment-1934879992
## [Codecov](https://app.codecov.io/gh/apache/pinot/pull/12384?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
Attention: `16 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`7c9bf8c`)](https://app.codecov.io/gh/apache/pinot/commit/7c9bf8cf49b0ed78f1de26edffa602bd3f74f548?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 61.77% compared to head [(`0546c8e`)](https://app.codecov.io/gh/apache/pinot/pull/12384?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) 27.73%.
| [Files](https://app.codecov.io/gh/apache/pinot/pull/12384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [...n/java/org/apache/pinot/common/utils/TlsUtils.java](https://app.codecov.io/gh/apache/pinot/pull/12384?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGlub3QtY29tbW9uL3NyYy9tYWluL2phdmEvb3JnL2FwYWNoZS9waW5vdC9jb21tb24vdXRpbHMvVGxzVXRpbHMuamF2YQ==) | 0.00% | [16 Missing :warning: ](https://app.codecov.io/gh/apache/pinot/pull/12384?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 #12384 +/- ##
=============================================
- Coverage 61.77% 27.73% -34.04%
Complexity 207 207
=============================================
Files 2425 2425
Lines 132753 132766 +13
Branches 20535 20535
=============================================
- Hits 82005 36821 -45184
- Misses 44730 93150 +48420
+ Partials 6018 2795 -3223
```
| [Flag](https://app.codecov.io/gh/apache/pinot/pull/12384/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/12384/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/12384/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/12384/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%> (ø)` | |
| [integration2](https://app.codecov.io/gh/apache/pinot/pull/12384/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/12384/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%> (-34.96%)` | :arrow_down: |
| [java-21](https://app.codecov.io/gh/apache/pinot/pull/12384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.73% <0.00%> (-33.91%)` | :arrow_down: |
| [skip-bytebuffers-false](https://app.codecov.io/gh/apache/pinot/pull/12384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.73% <0.00%> (-34.00%)` | :arrow_down: |
| [skip-bytebuffers-true](https://app.codecov.io/gh/apache/pinot/pull/12384/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%> (-61.61%)` | :arrow_down: |
| [temurin](https://app.codecov.io/gh/apache/pinot/pull/12384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.73% <0.00%> (-34.04%)` | :arrow_down: |
| [unittests](https://app.codecov.io/gh/apache/pinot/pull/12384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <0.00%> (-34.04%)` | :arrow_down: |
| [unittests1](https://app.codecov.io/gh/apache/pinot/pull/12384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
| [unittests2](https://app.codecov.io/gh/apache/pinot/pull/12384/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `27.72% <0.00%> (-0.04%)` | :arrow_down: |
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/12384?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