[GitHub] [hadoop-ozone] elek commented on a change in pull request #1096: HDDS-3833. Use Pipeline choose policy to choose pipeline from exist pipeline list

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

elek commented on a change in pull request #1096:
URL: https://github.com/apache/hadoop-ozone/pull/1096#discussion_r460759668



##########
File path: hadoop-hdds/common/src/main/resources/ozone-default.xml
##########
@@ -814,6 +814,20 @@
       value.
     </description>
   </property>
+  <property>

Review comment:
       I would prefer to use the new, Java based configuration API:
   
   https://cwiki.apache.org/confluence/display/HADOOP/Java-based+configuration+API

##########
File path: hadoop-hdds/interface-server/src/main/resources/proto.lock
##########
@@ -1209,6 +1209,10 @@
               {
                 "name": "INTERNAL_ERROR",
                 "integer": 29
+              },

Review comment:
       Proto.lock file change should be excluded. the `proto.lock` should represent and old state: the last released state to use it as a base for comparison.
   
   Rebase to the latest master and you won't see the changed proto lock files (which makes it easy to add lock files accidentally) 

##########
File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java
##########
@@ -222,9 +224,11 @@ public AllocatedBlock allocateBlock(final long size, ReplicationType type,
       }
 
       if (null == pipeline) {
-        // TODO: #CLUTIL Make the selection policy driven.
-        pipeline = availablePipelines
-            .get((int) (Math.random() * availablePipelines.size()));
+        Map<String, Object> params = new HashMap<>();
+        params.put(
+            PipelineChoosePolicy.PIPELINE_CHOOSE_POLICY_PARAM_SIZE, size);
+        pipeline = pipelineChoosePolicy.choosePipeline(

Review comment:
       It seems to be a standard parameter which is added to all the implementation. Wouldn't be better to add it as a type-safe field? Or we can use a `PipelineRequestInformation` class instead of the `Map<String,Object>` if you would like to make it easier to add more information latest.




----------------------------------------------------------------
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:
users@infra.apache.org



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