[GitHub] [ozone] szetszwo commented on a change in pull request #2452: HDDS-5480. [Ozone-Streaming] Client and server should support stream setup.

[GitBox <gi...@apache.org>]

szetszwo commented on a change in pull request #2452:
URL: https://github.com/apache/ozone/pull/2452#discussion_r677143241



##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -79,6 +85,18 @@
       "dfs.container.ratis.server.port";
   public static final int DFS_CONTAINER_RATIS_SERVER_PORT_DEFAULT = 9856;
 
+  /**
+   * Ratis Port where containers listen to datastream requests.
+   */
+  public static final String DFS_CONTAINER_RATIS_DATASTREAM_ENABLE
+      = "dfs.container.ratis.datastream.enable";
+  public static final boolean DFS_CONTAINER_RATIS_DATASTREAM_ENABLE_DEFAULT
+      = true;
+  public static final String DFS_CONTAINER_RATIS_DATASTREAM_IPC_PORT
+      = "dfs.container.ratis.datastream.ipc";

Review comment:
       Let's rename this to "dfs.container.ratis.datastream.port".

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java
##########
@@ -774,7 +774,8 @@ public static Port newPort(Port.Name name, Integer value) {
      * Ports that are supported in DataNode.
      */
     public enum Name {
-      STANDALONE, RATIS, REST, REPLICATION, RATIS_ADMIN, RATIS_SERVER;
+      STANDALONE, RATIS, REST, REPLICATION, RATIS_ADMIN, RATIS_SERVER,
+      DATASTREAM;

Review comment:
       Let's call it RATIS_DATASTREAM.

##########
File path: hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/XceiverClientGrpc.java
##########
@@ -283,6 +284,14 @@ public ContainerCommandResponseProto sendCommand(
     return responseProtoHashMap;
   }
 
+  // TODO: We need separate XceiverClientRatis and XceiverClientGrpc instances
+  //  and remove XceiverClientSpi
+  @Override
+  public DataStreamApi getDataStreamApi() {
+    throw new UnsupportedOperationException(
+        "Operation Not supported for XceiverClientGrpc");
+  }

Review comment:
       getDataStreamApi is yet not used.  Let's add it later when it is being called.
   
   BTW, we may only add the method to XceiverClientRatis and cast the object for calling it as below.
   ```
   XceiverClientSpi client = ...
   DataStreamApi streamApi = ((XceiverClientRatis)client).getDataStreamApi();
   ```

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConfigKeys.java
##########
@@ -57,6 +57,12 @@
   public static final boolean DFS_CONTAINER_IPC_RANDOM_PORT_DEFAULT =
       false;
 
+  public static final String DFS_CONTAINER_RATIS_DATASTREAM_IPC_RANDOM_PORT =
+      "dfs.container.ratis.datastream.ipc.random.port";

Review comment:
       Let's remove "ipc", i.e. dfs.container.ratis.datastream.random.port".  It is not really an ipc.

##########
File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/ratis/RatisHelper.java
##########
@@ -170,8 +172,12 @@ public static RaftGroup newRaftGroup(Pipeline pipeline) {
   public static RaftClient newRaftClient(RpcType rpcType, Pipeline pipeline,
       RetryPolicy retryPolicy, GrpcTlsConfig tlsConfig,
       ConfigurationSource ozoneConfiguration) throws IOException {
+    // TODO: For now, always set leader as primary. In the future we should
+    //  use the local or closest node in the pipline.
+    final DatanodeDetails leader = pipeline.getLeaderNode();
     return newRaftClient(rpcType,
-        toRaftPeerId(pipeline.getLeaderNode()),
+        toRaftPeerId(leader),
+        toRaftPeer(leader),

Review comment:
       Let's use the first node:
   ```
           toRaftPeer(pipeline.getFirstNode()),
   ```
   

##########
File path: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java
##########
@@ -177,6 +184,7 @@ private XceiverServerRatis(DatanodeDetails dd,
         HddsConfigKeys.HDDS_DATANODE_RATIS_SERVER_REQUEST_TIMEOUT,
         HddsConfigKeys.HDDS_DATANODE_RATIS_SERVER_REQUEST_TIMEOUT_DEFAULT,
         TimeUnit.MILLISECONDS);
+

Review comment:
       Let's revert this whitespace change.




-- 
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@ozone.apache.org

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



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