You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by GitBox <gi...@apache.org> on 2022/11/10 21:41:06 UTC
[GitHub] [hadoop] K0K0V0K commented on a diff in pull request #5115: YARN-10005. Code improvements in MutableCSConfigurationProvider
K0K0V0K commented on code in PR #5115:
URL: https://github.com/apache/hadoop/pull/5115#discussion_r1019610692
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ORDERING_POLICY;
+
+public final class ConfigurationUpdateAssembler {
+
+ private ConfigurationUpdateAssembler() {
+ }
+
+ public static Map<String, String> constructKeyValueConfUpdate(
+ CapacitySchedulerConfiguration proposedConf,
+ SchedConfUpdateInfo mutationInfo) throws IOException {
+
+ Map<String, String> confUpdate = new HashMap<>();
+ for (String queueToRemove : mutationInfo.getRemoveQueueInfo()) {
+ removeQueue(queueToRemove, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo addQueueInfo : mutationInfo.getAddQueueInfo()) {
+ addQueue(addQueueInfo, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo updateQueueInfo : mutationInfo.getUpdateQueueInfo()) {
+ updateQueue(updateQueueInfo, proposedConf, confUpdate);
+ }
+ for (Map.Entry<String, String> global : mutationInfo.getGlobalParams()
+ .entrySet()) {
+ confUpdate.put(global.getKey(), global.getValue());
+ }
+ return confUpdate;
+ }
+
+ private static void removeQueue(
+ String queueToRemove, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (queueToRemove != null) {
+ String queueName = queueToRemove.substring(
Review Comment:
I think that would be better if first we check the `queueToRemove` contains dot or not and we create the `queueName` after that check.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ORDERING_POLICY;
+
+public final class ConfigurationUpdateAssembler {
+
+ private ConfigurationUpdateAssembler() {
+ }
+
+ public static Map<String, String> constructKeyValueConfUpdate(
+ CapacitySchedulerConfiguration proposedConf,
+ SchedConfUpdateInfo mutationInfo) throws IOException {
+
+ Map<String, String> confUpdate = new HashMap<>();
+ for (String queueToRemove : mutationInfo.getRemoveQueueInfo()) {
+ removeQueue(queueToRemove, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo addQueueInfo : mutationInfo.getAddQueueInfo()) {
+ addQueue(addQueueInfo, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo updateQueueInfo : mutationInfo.getUpdateQueueInfo()) {
+ updateQueue(updateQueueInfo, proposedConf, confUpdate);
+ }
+ for (Map.Entry<String, String> global : mutationInfo.getGlobalParams()
+ .entrySet()) {
+ confUpdate.put(global.getKey(), global.getValue());
+ }
+ return confUpdate;
+ }
+
+ private static void removeQueue(
+ String queueToRemove, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (queueToRemove != null) {
+ String queueName = queueToRemove.substring(
+ queueToRemove.lastIndexOf('.') + 1);
+ if (queueToRemove.lastIndexOf('.') == -1) {
+ throw new IOException("Can't remove queue " + queueToRemove);
+ } else {
+ List<String> siblingQueues = getSiblingQueues(queueToRemove,
+ proposedConf);
+ if (!siblingQueues.contains(queueName)) {
+ throw new IOException("Queue " + queueToRemove + " not found");
+ }
+ siblingQueues.remove(queueName);
+ String parentQueuePath = queueToRemove.substring(0, queueToRemove
+ .lastIndexOf('.'));
+ proposedConf.setQueues(parentQueuePath, siblingQueues.toArray(
+ new String[0]));
+ String queuesConfig = CapacitySchedulerConfiguration.PREFIX
+ + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + CapacitySchedulerConfiguration.QUEUES;
+ if (siblingQueues.size() == 0) {
Review Comment:
I think isEmpty is better to use instead of size == 0, cause some List implementation requires O(N) to count the size, but can compute the isEmpty at O(1)
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ORDERING_POLICY;
+
+public final class ConfigurationUpdateAssembler {
+
+ private ConfigurationUpdateAssembler() {
+ }
+
+ public static Map<String, String> constructKeyValueConfUpdate(
+ CapacitySchedulerConfiguration proposedConf,
+ SchedConfUpdateInfo mutationInfo) throws IOException {
+
+ Map<String, String> confUpdate = new HashMap<>();
+ for (String queueToRemove : mutationInfo.getRemoveQueueInfo()) {
+ removeQueue(queueToRemove, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo addQueueInfo : mutationInfo.getAddQueueInfo()) {
+ addQueue(addQueueInfo, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo updateQueueInfo : mutationInfo.getUpdateQueueInfo()) {
+ updateQueue(updateQueueInfo, proposedConf, confUpdate);
+ }
+ for (Map.Entry<String, String> global : mutationInfo.getGlobalParams()
+ .entrySet()) {
+ confUpdate.put(global.getKey(), global.getValue());
+ }
+ return confUpdate;
+ }
+
+ private static void removeQueue(
+ String queueToRemove, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (queueToRemove != null) {
Review Comment:
If we use an `if (queueToRemove == null) {return;}` like early return, we can reduce the depth of this method
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ORDERING_POLICY;
+
+public final class ConfigurationUpdateAssembler {
+
+ private ConfigurationUpdateAssembler() {
+ }
+
+ public static Map<String, String> constructKeyValueConfUpdate(
+ CapacitySchedulerConfiguration proposedConf,
+ SchedConfUpdateInfo mutationInfo) throws IOException {
+
+ Map<String, String> confUpdate = new HashMap<>();
+ for (String queueToRemove : mutationInfo.getRemoveQueueInfo()) {
+ removeQueue(queueToRemove, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo addQueueInfo : mutationInfo.getAddQueueInfo()) {
+ addQueue(addQueueInfo, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo updateQueueInfo : mutationInfo.getUpdateQueueInfo()) {
+ updateQueue(updateQueueInfo, proposedConf, confUpdate);
+ }
+ for (Map.Entry<String, String> global : mutationInfo.getGlobalParams()
+ .entrySet()) {
+ confUpdate.put(global.getKey(), global.getValue());
+ }
+ return confUpdate;
+ }
+
+ private static void removeQueue(
+ String queueToRemove, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (queueToRemove != null) {
+ String queueName = queueToRemove.substring(
+ queueToRemove.lastIndexOf('.') + 1);
+ if (queueToRemove.lastIndexOf('.') == -1) {
+ throw new IOException("Can't remove queue " + queueToRemove);
+ } else {
Review Comment:
Minor stuff, but we can leave this else, cause in the if we throw an exception, so the run will be interrupted.
This way we can reduce the depth of the code.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ORDERING_POLICY;
+
+public final class ConfigurationUpdateAssembler {
+
+ private ConfigurationUpdateAssembler() {
+ }
+
+ public static Map<String, String> constructKeyValueConfUpdate(
+ CapacitySchedulerConfiguration proposedConf,
+ SchedConfUpdateInfo mutationInfo) throws IOException {
+
+ Map<String, String> confUpdate = new HashMap<>();
+ for (String queueToRemove : mutationInfo.getRemoveQueueInfo()) {
+ removeQueue(queueToRemove, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo addQueueInfo : mutationInfo.getAddQueueInfo()) {
+ addQueue(addQueueInfo, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo updateQueueInfo : mutationInfo.getUpdateQueueInfo()) {
+ updateQueue(updateQueueInfo, proposedConf, confUpdate);
+ }
+ for (Map.Entry<String, String> global : mutationInfo.getGlobalParams()
+ .entrySet()) {
+ confUpdate.put(global.getKey(), global.getValue());
+ }
+ return confUpdate;
+ }
+
+ private static void removeQueue(
+ String queueToRemove, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (queueToRemove != null) {
+ String queueName = queueToRemove.substring(
+ queueToRemove.lastIndexOf('.') + 1);
+ if (queueToRemove.lastIndexOf('.') == -1) {
+ throw new IOException("Can't remove queue " + queueToRemove);
+ } else {
+ List<String> siblingQueues = getSiblingQueues(queueToRemove,
+ proposedConf);
+ if (!siblingQueues.contains(queueName)) {
+ throw new IOException("Queue " + queueToRemove + " not found");
+ }
+ siblingQueues.remove(queueName);
+ String parentQueuePath = queueToRemove.substring(0, queueToRemove
+ .lastIndexOf('.'));
+ proposedConf.setQueues(parentQueuePath, siblingQueues.toArray(
+ new String[0]));
+ String queuesConfig = CapacitySchedulerConfiguration.PREFIX
+ + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + CapacitySchedulerConfiguration.QUEUES;
+ if (siblingQueues.size() == 0) {
+ confUpdate.put(queuesConfig, null);
+ // Unset Ordering Policy of Leaf Queue converted from
+ // Parent Queue after removeQueue
+ String queueOrderingPolicy = CapacitySchedulerConfiguration.PREFIX
+ + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + ORDERING_POLICY;
+ proposedConf.unset(queueOrderingPolicy);
+ confUpdate.put(queueOrderingPolicy, null);
+ } else {
+ confUpdate.put(queuesConfig, Joiner.on(',').join(siblingQueues));
+ }
+ for (Map.Entry<String, String> confRemove : proposedConf.getValByRegex(
+ ".*" + queueToRemove.replaceAll("\\.", "\\.") + "\\..*")
Review Comment:
why do we need this replace all?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/TestConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,170 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertThrows;
+
+/**
+ * Tests {@link ConfigurationUpdateAssembler}.
+ */
+public class TestConfigurationUpdateAssembler {
+
+ private static final String A_PATH = "root.a";
+ private static final String B_PATH = "root.b";
+ private static final String C_PATH = "root.c";
+
+ private static final String CONFIG_NAME = "testConfigName";
+ private static final String A_CONFIG_PATH = CapacitySchedulerConfiguration.PREFIX + A_PATH +
+ CapacitySchedulerConfiguration.DOT + CONFIG_NAME;
+ private static final String B_CONFIG_PATH = CapacitySchedulerConfiguration.PREFIX + B_PATH +
+ CapacitySchedulerConfiguration.DOT + CONFIG_NAME;
+ private static final String C_CONFIG_PATH = CapacitySchedulerConfiguration.PREFIX + C_PATH +
+ CapacitySchedulerConfiguration.DOT + CONFIG_NAME;
+ private static final String ROOT_QUEUES_PATH = CapacitySchedulerConfiguration.PREFIX +
+ CapacitySchedulerConfiguration.ROOT + CapacitySchedulerConfiguration.DOT +
+ CapacitySchedulerConfiguration.QUEUES;
+
+ private static final String A_INIT_CONFIG_VALUE = "aInitValue";
+ private static final String A_CONFIG_VALUE = "aValue";
+ private static final String B_INIT_CONFIG_VALUE = "bInitValue";
+ private static final String B_CONFIG_VALUE = "bValue";
+ private static final String C_CONFIG_VALUE = "cValue";
+
+ private CapacitySchedulerConfiguration csConfig;
+
+ @Before
+ public void setUp() {
+ csConfig = crateInitialCSConfig();
+ }
+
+ @Test
+ public void testAddQueue() throws Exception {
+ SchedConfUpdateInfo updateInfo = new SchedConfUpdateInfo();
+ Map<String, String> updateMap = new HashMap<>();
+ updateMap.put(CONFIG_NAME, C_CONFIG_VALUE);
+ QueueConfigInfo queueConfigInfo = new QueueConfigInfo(C_PATH, updateMap);
+ updateInfo.getAddQueueInfo().add(queueConfigInfo);
+
+ Map<String, String> configurationUpdate =
+ ConfigurationUpdateAssembler.constructKeyValueConfUpdate(csConfig, updateInfo);
+
+ assertEquals(C_CONFIG_VALUE, configurationUpdate.get(C_CONFIG_PATH));
+ assertEquals("a,b,c", configurationUpdate.get(ROOT_QUEUES_PATH));
+ }
+
+ @Test
+ public void testAddExistingQueue() {
+ SchedConfUpdateInfo updateInfo = new SchedConfUpdateInfo();
+ Map<String, String> updateMap = new HashMap<>();
+ updateMap.put(CONFIG_NAME, A_CONFIG_VALUE);
+ QueueConfigInfo queueConfigInfo = new QueueConfigInfo(A_PATH, updateMap);
+ updateInfo.getAddQueueInfo().add(queueConfigInfo);
+
+ assertThrows(IOException.class, () -> {
+ ConfigurationUpdateAssembler.constructKeyValueConfUpdate(csConfig, updateInfo);
+ });
+ }
+
+ @Test
+ public void testAddInvalidQueue() {
+ SchedConfUpdateInfo updateInfo = new SchedConfUpdateInfo();
+ Map<String, String> updateMap = new HashMap<>();
+ updateMap.put(CONFIG_NAME, A_CONFIG_VALUE);
+ QueueConfigInfo queueConfigInfo = new QueueConfigInfo("invalidPath", updateMap);
+ updateInfo.getAddQueueInfo().add(queueConfigInfo);
+
+ assertThrows(IOException.class, () -> {
+ ConfigurationUpdateAssembler.constructKeyValueConfUpdate(csConfig, updateInfo);
+ });
+ }
+
+ @Test
+ public void testUpdateQueue() throws Exception {
+ SchedConfUpdateInfo updateInfo = new SchedConfUpdateInfo();
+ Map<String, String> updateMap = new HashMap<>();
+ updateMap.put(CONFIG_NAME, A_CONFIG_VALUE);
+ QueueConfigInfo queueAConfigInfo = new QueueConfigInfo(A_PATH, updateMap);
+ updateInfo.getUpdateQueueInfo().add(queueAConfigInfo);
+
+ Map<String, String> updateMapQueueB = new HashMap<>();
+ updateMapQueueB.put(CONFIG_NAME, B_CONFIG_VALUE);
+ QueueConfigInfo queueBConfigInfo = new QueueConfigInfo(B_PATH, updateMapQueueB);
+
+ updateInfo.getUpdateQueueInfo().add(queueBConfigInfo);
+
+ Map<String, String> configurationUpdate =
+ ConfigurationUpdateAssembler.constructKeyValueConfUpdate(csConfig, updateInfo);
+
+ assertEquals(A_CONFIG_VALUE, configurationUpdate.get(A_CONFIG_PATH));
+ assertEquals(B_CONFIG_VALUE, configurationUpdate.get(B_CONFIG_PATH));
+ }
+
+ @Test
+ public void testRemoveQueue() throws Exception {
+ SchedConfUpdateInfo updateInfo = new SchedConfUpdateInfo();
+ updateInfo.getRemoveQueueInfo().add(A_PATH);
+
+ Map<String, String> configurationUpdate =
+ ConfigurationUpdateAssembler.constructKeyValueConfUpdate(csConfig, updateInfo);
+
+ assertEquals(null, configurationUpdate.get(A_CONFIG_PATH));
Review Comment:
I think assertNull is more meaning full
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ORDERING_POLICY;
+
+public final class ConfigurationUpdateAssembler {
+
+ private ConfigurationUpdateAssembler() {
+ }
+
+ public static Map<String, String> constructKeyValueConfUpdate(
+ CapacitySchedulerConfiguration proposedConf,
+ SchedConfUpdateInfo mutationInfo) throws IOException {
+
+ Map<String, String> confUpdate = new HashMap<>();
+ for (String queueToRemove : mutationInfo.getRemoveQueueInfo()) {
+ removeQueue(queueToRemove, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo addQueueInfo : mutationInfo.getAddQueueInfo()) {
+ addQueue(addQueueInfo, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo updateQueueInfo : mutationInfo.getUpdateQueueInfo()) {
+ updateQueue(updateQueueInfo, proposedConf, confUpdate);
+ }
+ for (Map.Entry<String, String> global : mutationInfo.getGlobalParams()
+ .entrySet()) {
+ confUpdate.put(global.getKey(), global.getValue());
+ }
+ return confUpdate;
+ }
+
+ private static void removeQueue(
+ String queueToRemove, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (queueToRemove != null) {
+ String queueName = queueToRemove.substring(
+ queueToRemove.lastIndexOf('.') + 1);
+ if (queueToRemove.lastIndexOf('.') == -1) {
+ throw new IOException("Can't remove queue " + queueToRemove);
+ } else {
+ List<String> siblingQueues = getSiblingQueues(queueToRemove,
+ proposedConf);
+ if (!siblingQueues.contains(queueName)) {
+ throw new IOException("Queue " + queueToRemove + " not found");
+ }
+ siblingQueues.remove(queueName);
+ String parentQueuePath = queueToRemove.substring(0, queueToRemove
+ .lastIndexOf('.'));
+ proposedConf.setQueues(parentQueuePath, siblingQueues.toArray(
+ new String[0]));
+ String queuesConfig = CapacitySchedulerConfiguration.PREFIX
+ + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + CapacitySchedulerConfiguration.QUEUES;
+ if (siblingQueues.size() == 0) {
+ confUpdate.put(queuesConfig, null);
+ // Unset Ordering Policy of Leaf Queue converted from
+ // Parent Queue after removeQueue
+ String queueOrderingPolicy = CapacitySchedulerConfiguration.PREFIX
+ + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + ORDERING_POLICY;
+ proposedConf.unset(queueOrderingPolicy);
+ confUpdate.put(queueOrderingPolicy, null);
+ } else {
+ confUpdate.put(queuesConfig, Joiner.on(',').join(siblingQueues));
+ }
+ for (Map.Entry<String, String> confRemove : proposedConf.getValByRegex(
+ ".*" + queueToRemove.replaceAll("\\.", "\\.") + "\\..*")
+ .entrySet()) {
+ proposedConf.unset(confRemove.getKey());
+ confUpdate.put(confRemove.getKey(), null);
+ }
+ }
+ }
+ }
+
+ private static void addQueue(
+ QueueConfigInfo addInfo, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (addInfo != null) {
+ String queuePath = addInfo.getQueue();
+ String queueName = queuePath.substring(queuePath.lastIndexOf('.') + 1);
+ if (queuePath.lastIndexOf('.') == -1) {
+ throw new IOException("Can't add invalid queue " + queuePath);
+ } else if (getSiblingQueues(queuePath, proposedConf).contains(
+ queueName)) {
+ throw new IOException("Can't add existing queue " + queuePath);
+ }
+ String parentQueue = queuePath.substring(0, queuePath.lastIndexOf('.'));
+ String[] siblings = proposedConf.getQueues(parentQueue);
+ List<String> siblingQueues = siblings == null ? new ArrayList<>() :
+ new ArrayList<>(Arrays.<String>asList(siblings));
Review Comment:
Do we need this `<String>` here, or we can use just `new ArrayList<>(Arrays.asList(siblings))`
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/conf/ConfigurationUpdateAssembler.java:
##########
@@ -0,0 +1,177 @@
+/**
+ * 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.yarn.server.resourcemanager.scheduler.capacity.conf;
+
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.thirdparty.com.google.common.base.Joiner;
+import org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration;
+import org.apache.hadoop.yarn.webapp.dao.QueueConfigInfo;
+import org.apache.hadoop.yarn.webapp.dao.SchedConfUpdateInfo;
+
+import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+
+import static org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.CapacitySchedulerConfiguration.ORDERING_POLICY;
+
+public final class ConfigurationUpdateAssembler {
+
+ private ConfigurationUpdateAssembler() {
+ }
+
+ public static Map<String, String> constructKeyValueConfUpdate(
+ CapacitySchedulerConfiguration proposedConf,
+ SchedConfUpdateInfo mutationInfo) throws IOException {
+
+ Map<String, String> confUpdate = new HashMap<>();
+ for (String queueToRemove : mutationInfo.getRemoveQueueInfo()) {
+ removeQueue(queueToRemove, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo addQueueInfo : mutationInfo.getAddQueueInfo()) {
+ addQueue(addQueueInfo, proposedConf, confUpdate);
+ }
+ for (QueueConfigInfo updateQueueInfo : mutationInfo.getUpdateQueueInfo()) {
+ updateQueue(updateQueueInfo, proposedConf, confUpdate);
+ }
+ for (Map.Entry<String, String> global : mutationInfo.getGlobalParams()
+ .entrySet()) {
+ confUpdate.put(global.getKey(), global.getValue());
+ }
+ return confUpdate;
+ }
+
+ private static void removeQueue(
+ String queueToRemove, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (queueToRemove != null) {
+ String queueName = queueToRemove.substring(
+ queueToRemove.lastIndexOf('.') + 1);
+ if (queueToRemove.lastIndexOf('.') == -1) {
+ throw new IOException("Can't remove queue " + queueToRemove);
+ } else {
+ List<String> siblingQueues = getSiblingQueues(queueToRemove,
+ proposedConf);
+ if (!siblingQueues.contains(queueName)) {
+ throw new IOException("Queue " + queueToRemove + " not found");
+ }
+ siblingQueues.remove(queueName);
+ String parentQueuePath = queueToRemove.substring(0, queueToRemove
+ .lastIndexOf('.'));
+ proposedConf.setQueues(parentQueuePath, siblingQueues.toArray(
+ new String[0]));
+ String queuesConfig = CapacitySchedulerConfiguration.PREFIX
+ + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + CapacitySchedulerConfiguration.QUEUES;
+ if (siblingQueues.size() == 0) {
+ confUpdate.put(queuesConfig, null);
+ // Unset Ordering Policy of Leaf Queue converted from
+ // Parent Queue after removeQueue
+ String queueOrderingPolicy = CapacitySchedulerConfiguration.PREFIX
+ + parentQueuePath + CapacitySchedulerConfiguration.DOT
+ + ORDERING_POLICY;
+ proposedConf.unset(queueOrderingPolicy);
+ confUpdate.put(queueOrderingPolicy, null);
+ } else {
+ confUpdate.put(queuesConfig, Joiner.on(',').join(siblingQueues));
+ }
+ for (Map.Entry<String, String> confRemove : proposedConf.getValByRegex(
+ ".*" + queueToRemove.replaceAll("\\.", "\\.") + "\\..*")
+ .entrySet()) {
+ proposedConf.unset(confRemove.getKey());
+ confUpdate.put(confRemove.getKey(), null);
+ }
+ }
+ }
+ }
+
+ private static void addQueue(
+ QueueConfigInfo addInfo, CapacitySchedulerConfiguration proposedConf,
+ Map<String, String> confUpdate) throws IOException {
+ if (addInfo != null) {
+ String queuePath = addInfo.getQueue();
+ String queueName = queuePath.substring(queuePath.lastIndexOf('.') + 1);
+ if (queuePath.lastIndexOf('.') == -1) {
+ throw new IOException("Can't add invalid queue " + queuePath);
+ } else if (getSiblingQueues(queuePath, proposedConf).contains(
+ queueName)) {
+ throw new IOException("Can't add existing queue " + queuePath);
+ }
+ String parentQueue = queuePath.substring(0, queuePath.lastIndexOf('.'));
+ String[] siblings = proposedConf.getQueues(parentQueue);
+ List<String> siblingQueues = siblings == null ? new ArrayList<>() :
+ new ArrayList<>(Arrays.<String>asList(siblings));
+ siblingQueues.add(queuePath.substring(queuePath.lastIndexOf('.') + 1));
+ proposedConf.setQueues(parentQueue,
+ siblingQueues.toArray(new String[0]));
+ confUpdate.put(CapacitySchedulerConfiguration.PREFIX
+ + parentQueue + CapacitySchedulerConfiguration.DOT
+ + CapacitySchedulerConfiguration.QUEUES,
+ Joiner.on(',').join(siblingQueues));
+ String keyPrefix = CapacitySchedulerConfiguration.PREFIX
+ + queuePath + CapacitySchedulerConfiguration.DOT;
+ for (Map.Entry<String, String> kv : addInfo.getParams().entrySet()) {
+ if (kv.getValue() == null) {
Review Comment:
In the updateQueue method we check the the value is empty or not. Why we dont check that here?
--
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: common-issues-unsubscribe@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org