GlenGeng commented on a change in pull request #1371:
URL: https://github.com/apache/hadoop-ozone/pull/1371#discussion_r486090313



##########
File path: 
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/ratis/RatisHelper.java
##########
@@ -125,6 +131,17 @@ private static RaftGroup newRaftGroup(Collection<RaftPeer> 
peers) {
         : RaftGroup.valueOf(DUMMY_GROUP_ID, peers);
   }
 
+  public static RaftGroup newRaftGroup(RaftGroupId groupId,
+      List<DatanodeDetails> peers, List<Integer> priorityList) {
+    final List<RaftPeer> newPeers = new ArrayList<>();

Review comment:
       sanity check for `peers.size() == priorityList.size()`

##########
File path: 
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/protocol/DatanodeDetails.java
##########
@@ -190,6 +195,18 @@ public Port getPort(Port.Name name) {
     return null;
   }
 
+  public int getSuggestedLeaderCount() {

Review comment:
       miss java doc for these public method.

##########
File path: 
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java
##########
@@ -123,6 +124,14 @@ public Instant getCreationTimestamp() {
     return creationTimestamp;
   }
 
+  public void setSuggestedLeader(UUID suggestedLeader) {

Review comment:
       miss java doc for public method.
   
   Why not set suggestedLeader in Ctor and remove the getter/setter ? I guess, 
it would be final/immutable after creating, just like type/factor.

##########
File path: 
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/protocol/commands/CreatePipelineCommand.java
##########
@@ -48,16 +50,34 @@ public CreatePipelineCommand(final PipelineID pipelineID,
     this.factor = factor;
     this.type = type;
     this.nodelist = datanodeList;
+    this.priorityList = new ArrayList<>();
+    for (DatanodeDetails dn : datanodeList) {

Review comment:
       ditto

##########
File path: 
hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/pipeline/Pipeline.java
##########
@@ -61,6 +61,7 @@
   private UUID leaderId;
   // Timestamp for pipeline upon creation
   private Instant creationTimestamp;
+  private UUID suggestedLeader;

Review comment:
       how about suggestedLeaderId ?

##########
File path: 
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/CreatePipelineCommandHandler.java
##########
@@ -82,18 +104,22 @@ public void handle(SCMCommand command, OzoneContainer 
ozoneContainer,
     final CreatePipelineCommandProto createCommand =
         ((CreatePipelineCommand)command).getProto();
     final HddsProtos.PipelineID pipelineID = createCommand.getPipelineID();
-    final Collection<DatanodeDetails> peers =
+    final List<DatanodeDetails> peers =
         createCommand.getDatanodeList().stream()
             .map(DatanodeDetails::getFromProtoBuf)
             .collect(Collectors.toList());
+    final List<Integer> priorityList = createCommand.getPriorityList();
+
+    incSuggestedLeaderCount(priorityList, peers, dn);

Review comment:
       increase the counter after successfully created the pipeline ? Might 
throw exception during the creation.

##########
File path: 
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/XceiverServerRatis.java
##########
@@ -711,10 +711,23 @@ private long 
calculatePipelineBytesWritten(HddsProtos.PipelineID pipelineID) {
 
   @Override
   public void addGroup(HddsProtos.PipelineID pipelineId,
-      Collection<DatanodeDetails> peers) throws IOException {
+      List<DatanodeDetails> peers) throws IOException {
+    List<Integer> priorityList = new ArrayList<>();

Review comment:
       ``
   List<Integer> priorityList = new ArrayList<>(peers.size());
   for (...)
   ``
   
   so that won't be bothered by unused var `dn`

##########
File path: 
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/RatisPipelineProvider.java
##########
@@ -98,8 +102,40 @@ private boolean exceedPipelineNumberLimit(ReplicationFactor 
factor) {
     return false;
   }
 
+  private DatanodeDetails getSuggestedLeader(List<DatanodeDetails> dns) {
+    int minLeaderCount = Integer.MAX_VALUE;
+    DatanodeDetails suggestedLeader = null;
+
+    for (int i = 0; i < dns.size(); i++) {

Review comment:
       as suggested by idea, range for
   `for (dn : dns)`




----------------------------------------------------------------
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.

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



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

Reply via email to