mridulm commented on code in PR #2555:
URL: https://github.com/apache/celeborn/pull/2555#discussion_r1680348167
##########
common/src/main/java/org/apache/celeborn/common/network/ssl/ReloadingX509TrustManager.java:
##########
@@ -200,20 +200,22 @@ public void run() {
} catch (InterruptedException e) {
running = false;
}
- try {
- if (running && needsReload()) {
- try {
- trustManagerRef.set(loadTrustManager());
- this.reloadCount += 1;
- } catch (Exception ex) {
- logger.warn(
- "Could not load truststore (keep using existing one) : " +
ex.toString(), ex);
+ synchronized (this) {
Review Comment:
This change is not required IMO. These `volatile` variables being updated
only in this method.
Technically, they dont need to be `volatile` - but were made volatile only
to ensure we can mutate them from tests, and they become visible.
Note that if the intent is to actually address the corner case race
condition of mutation from the test (that is reasonable) - then we should wrap
the mutations from `ReloadingX509TrustManagerSuiteJ` as well in `synchronized`
##########
common/src/test/java/org/apache/celeborn/common/network/ssl/ReloadingX509TrustManagerSuiteJ.java:
##########
@@ -101,15 +101,7 @@ public void testLoadMissingTrustStore() throws Exception {
assertThrows(
IOException.class,
- () -> {
- ReloadingX509TrustManager tm =
- new ReloadingX509TrustManager(KeyStore.getDefaultType(),
trustStore, "password", 10);
- try {
- tm.init();
- } finally {
- tm.destroy();
- }
- });
+ () -> new ReloadingX509TrustManager(KeyStore.getDefaultType(),
trustStore, "password", 10));
Review Comment:
Ah, i see what you mean ... makes sense.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
