mridulm commented on code in PR #2460:
URL: https://github.com/apache/celeborn/pull/2460#discussion_r1568258966
##########
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java:
##########
@@ -21,6 +21,17 @@ public class TransportModuleConstants {
public static final String PUSH_MODULE = "push";
public static final String REPLICATE_MODULE = "replicate";
public static final String FETCH_MODULE = "fetch";
- public static final String RPC_MODULE = "rpc";
+
+ // RPC module used by the application components to communicate with each
other
+ // This is used only at the application side.
+ public static final String RPC_APP_MODULE = "rpc_app";
+ // RPC module used to communicate with/between server components
+ // This is used both at server (master/worker) and application side.
+ public static final String RPC_SERVER_MODULE = "rpc_server";
Review Comment:
Doesn't this not need to indicate it is specifically for rpc ? and not for
other transport modules ?
The module name also applies to configs - do we support `:` in config names
? (for example, `--conf spark.celeborn.app:servoce.io.connectionTimeout` will
run into issues with `:` parsing I would expect)
##########
common/src/main/java/org/apache/celeborn/common/protocol/TransportModuleConstants.java:
##########
@@ -21,6 +21,17 @@ public class TransportModuleConstants {
public static final String PUSH_MODULE = "push";
public static final String REPLICATE_MODULE = "replicate";
public static final String FETCH_MODULE = "fetch";
- public static final String RPC_MODULE = "rpc";
+
+ // RPC module used by the application components to communicate with each
other
+ // This is used only at the application side.
+ public static final String RPC_APP_MODULE = "rpc_app";
+ // RPC module used to communicate with/between server components
+ // This is used both at server (master/worker) and application side.
+ public static final String RPC_SERVER_MODULE = "rpc_server";
Review Comment:
Doesn't this not need to indicate it is specifically for rpc ? and not for
other transport modules ?
The module name also applies to configs - do we support `:` in config names
? (for example, `--conf spark.celeborn.app:service.io.connectionTimeout` will
run into issues with `:` parsing I would expect)
--
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]
