avijayanhwx commented on a change in pull request #1096: URL: https://github.com/apache/hadoop-ozone/pull/1096#discussion_r459577998
########## File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/PipelineChoosePolicy.java ########## @@ -0,0 +1,36 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with this + * work for additional information regarding copyright ownership. The ASF + * licenses this file to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package org.apache.hadoop.hdds.scm; + +import org.apache.hadoop.hdds.scm.pipeline.Pipeline; + +import java.util.List; + +/** + * A {@link PipelineChoosePolicy} support choosing pipeline from exist list. + */ +public interface PipelineChoosePolicy { + + /** + * Given an initial list of pipelines, return one of the pipelines. + * + * @param pipelineList list of pipelines. + * @return one of the pipelines. + */ + Pipeline choosePipeline(List<Pipeline> pipelineList); Review comment: @maobaolong For my knowledge, can you list a few pipeline selection policies that you envision? I am wondering if we are OK with just a List\<Pipeline\> as an argument. ########## File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/pipeline/choose/algorithms/PipelineChoosePolicyFactory.java ########## @@ -0,0 +1,71 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.hadoop.hdds.scm.pipeline.choose.algorithms; + +import org.apache.hadoop.hdds.conf.ConfigurationSource; +import org.apache.hadoop.hdds.scm.PipelineChoosePolicy; +import org.apache.hadoop.hdds.scm.ScmConfigKeys; +import org.apache.hadoop.hdds.scm.exceptions.SCMException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.lang.reflect.Constructor; + +/** + * A factory to create pipeline choose policy instance based on configuration + * property {@link ScmConfigKeys#OZONE_SCM_PIPELINE_CHOOSE_IMPL_KEY}. + */ +public final class PipelineChoosePolicyFactory { + private static final Logger LOG = + LoggerFactory.getLogger(PipelineChoosePolicyFactory.class); + + private static final Class<? extends PipelineChoosePolicy> + OZONE_SCM_PIPELINE_CHOOSE_IMPL_DEFAULT = + RandomPipelineChoosePolicy.class; + + private PipelineChoosePolicyFactory() { + } + + public static PipelineChoosePolicy getPolicy( + ConfigurationSource conf) throws SCMException { + final Class<? extends PipelineChoosePolicy> policyClass = conf + .getClass(ScmConfigKeys.OZONE_SCM_PIPELINE_CHOOSE_IMPL_KEY, + OZONE_SCM_PIPELINE_CHOOSE_IMPL_DEFAULT, + PipelineChoosePolicy.class); + Constructor<? extends PipelineChoosePolicy> constructor; + try { + constructor = policyClass.getDeclaredConstructor(); Review comment: We are relying on default constructor here. And we pass in only List\<Pipeline\> through the API. We may have policies that need more information like the topology, client address etc as well. Can we make sure we support them without change of interface later? ########## File path: hadoop-hdds/common/src/main/java/org/apache/hadoop/hdds/scm/ScmConfigKeys.java ########## @@ -287,6 +287,9 @@ public static final String OZONE_SCM_PIPELINE_OWNER_CONTAINER_COUNT = "ozone.scm.pipeline.owner.container.count"; public static final int OZONE_SCM_PIPELINE_OWNER_CONTAINER_COUNT_DEFAULT = 3; + // Pipeline choose policy: + public static final String OZONE_SCM_PIPELINE_CHOOSE_IMPL_KEY = + "ozone.scm.pipeline.choose.impl"; Review comment: nit. Suggest rename ozone.scm.pipeline.choose.impl --> ozone.scm.pipeline.choose.policy.impl ########## File path: hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/BlockManagerImpl.java ########## @@ -222,9 +224,7 @@ 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())); + pipeline = pipelineChoosePolicy.choosePipeline(availablePipelines); Review comment: We could get a runtime exception from a plugged in policy. Can we have to fallback to default (Random) on error after logging? ---------------------------------------------------------------- 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: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
