[GitHub] [incubator-uniffle] advancedxy commented on a diff in pull request #712: [#711] feat(netty): Add Netty port information for Shuffle Server

["advancedxy (via GitHub)" <gi...@apache.org>]

advancedxy commented on code in PR #712:
URL: https://github.com/apache/incubator-uniffle/pull/712#discussion_r1134817772


##########
common/src/main/java/org/apache/uniffle/common/ShuffleServerInfo.java:
##########
@@ -25,19 +25,28 @@ public class ShuffleServerInfo implements Serializable {
 
   private String host;
 
-  private int port;
+  private int grpcPort;
+
+  private int nettyPort = 0;

Review Comment:
   If we are going to support random port. I believe port=`0` has special meanings. 
   
   How about make the default value of nettyPort to be -1?



##########
common/src/main/java/org/apache/uniffle/common/ClientType.java:
##########
@@ -18,5 +18,6 @@
 package org.apache.uniffle.common;
 
 public enum ClientType {
-  GRPC
+  GRPC,
+  GRPC_NETTY

Review Comment:
   is `NETTY` more appropriate?



##########
coordinator/src/main/java/org/apache/uniffle/coordinator/ServerNode.java:
##########
@@ -40,6 +40,7 @@ public class ServerNode implements Comparable<ServerNode> {
   private boolean isHealthy;
   private final ServerStatus status;
   private Map<String, StorageInfo> storageInfo;
+  private int nettyPort = 0;

Review Comment:
   ditto.



##########
server/src/main/java/org/apache/uniffle/server/ShuffleServer.java:
##########
@@ -182,8 +183,13 @@ private void initialization() throws Exception {
     if (ip == null) {
       throw new RuntimeException("Couldn't acquire host Ip");
     }
-    port = shuffleServerConf.getInteger(ShuffleServerConf.RPC_SERVER_PORT);
-    id = ip + "-" + port;
+    grpcPort = shuffleServerConf.getInteger(ShuffleServerConf.RPC_SERVER_PORT);
+    nettyPort = shuffleServerConf.getInteger(ShuffleServerConf.NETTY_SERVER_PORT);
+    if (nettyPort > 0) {
+      id = ip + "-" + grpcPort + "-" + nettyPort;
+    } else {
+      id = ip + "-" + grpcPort;

Review Comment:
   The id should be the actual port if passed port is zero for random port binding.
   
   But this could be addressed in another PR.



##########
server/src/main/java/org/apache/uniffle/server/ShuffleServerConf.java:
##########
@@ -397,6 +397,13 @@ public class ShuffleServerConf extends RssBaseConf {
       .defaultValue(true)
       .withDescription("Whether shutdown the server after server is decommissioned");
 
+  public static final ConfigOption<Integer> NETTY_SERVER_PORT = ConfigOptions
+      .key("rss.server.netty.port")
+      .intType()
+      .checkValue(ConfigUtils.POSITIVE_INTEGER_VALIDATOR_2, "netty port must be positive")
+      .defaultValue(0)

Review Comment:
   how about make the default value -1?  And reserve `0` for later random binding.



-- 
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: issues-unsubscribe@uniffle.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@uniffle.apache.org
For additional commands, e-mail: issues-help@uniffle.apache.org