hasnain-db commented on code in PR #2469:
URL: https://github.com/apache/celeborn/pull/2469#discussion_r1572634733
##########
common/src/main/java/org/apache/celeborn/common/network/TransportContext.java:
##########
@@ -209,17 +209,27 @@ public TransportChannelHandler initializePipeline(
private SSLFactory createSslFactory() {
if (conf.sslEnabled()) {
+
Review Comment:
the check on line 213 confused me a little and I had to read the code a
little to be sure this would work. Could we refactor this slightly?
IMO there should be 2 separate blocks for readability:
1. "if SSL is enabled and auto SSL enabled"
2. "if SSL is enabled and auto SSL is not enabled"
The bundling of the keys are valid check is confusing. It works today but as
the code evolves, if we update that function we might break here.
In addition, the error message if keys are not present here could probably
use some improvement (though outside the scope of this PR potentially). Since
in this case we only care about keystore being present right now, we can be
more specific.
##########
common/src/main/java/org/apache/celeborn/common/network/util/TransportConf.java:
##########
@@ -236,4 +242,29 @@ public boolean sslEnabledAndKeysAreValid() {
// It's fine for the trust store to be missing, we would default to
trusting all.
return true;
}
+
+ public boolean autoSslEnabled() {
+ // auto_ssl_enable is supported only for RPC_APP_MODULE
+ if (!TransportModuleConstants.RPC_APP_MODULE.equals(module)) return false;
+
+ // auto ssl must be enabled, and there should be no keystore or trust
store is configured
Review Comment:
grammar nit: "is" is redundant 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
