brumi1024 commented on code in PR #3618:
URL: https://github.com/apache/hadoop/pull/3618#discussion_r1194856356
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestLeafQueue.java:
##########
@@ -427,6 +439,10 @@ public void testSingleQueueOneUserMetrics() throws
Exception {
when(csContext.getNumClusterNodes()).thenReturn(numNodes);
root.updateClusterResource(clusterResource,
new ResourceLimits(clusterResource));
+ CapacitySchedulerQueueCapacityHandler queueController =
+ new
CapacitySchedulerQueueCapacityHandler(rmContext.getNodeLabelManager());
+ queueController.updateRoot(root, clusterResource);
Review Comment:
Why is updateRoot a method of QueueController while updateChildren is static
with a QueueController parameter?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestLeafQueue.java:
##########
@@ -703,6 +719,10 @@ public void testSingleQueueWithOneUser() throws Exception {
when(csContext.getNumClusterNodes()).thenReturn(numNodes);
root.updateClusterResource(clusterResource,
new ResourceLimits(clusterResource));
+ CapacitySchedulerQueueCapacityHandler queueController =
+ new
CapacitySchedulerQueueCapacityHandler(rmContext.getNodeLabelManager());
+ queueController.updateRoot(root, clusterResource);
+ updateChildren(queueController, clusterResource, root);
Review Comment:
A method could be introduced for this, as this part is repeated quite a few
times.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestLeafQueue.java:
##########
@@ -5236,6 +5346,10 @@ public void testMaxApplicationsWithNodeLabels() throws
IOException {
cs.getCapacitySchedulerQueueManager().reinitConfiguredNodeLabels(conf);
cs.setMaxRunningAppsEnforcer(new CSMaxRunningAppsEnforcer(cs));
cs.reinitialize(conf, cs.getRMContext());
+ CapacitySchedulerQueueCapacityHandler queueController =
+ new
CapacitySchedulerQueueCapacityHandler(rmContext.getNodeLabelManager());
+ queueController.updateRoot(cs.getQueue("root"), Resource.newInstance(10 *
GB, 10));
Review Comment:
Where does 10 GB / 10 vcore come from? The setup method has: 100 * 16 * GB,
100 * 32 vcore
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/AbstractResourceUsage.java:
##########
@@ -27,6 +27,8 @@
import java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock;
import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;
+import org.jline.utils.Log;
Review Comment:
Nit: I'm guessing this was left here by accident.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/ApplicationMasterServiceTestBase.java:
##########
@@ -453,6 +455,7 @@ public void testResourceTypes() throws Exception {
MockRM rm = new MockRM(entry.getKey());
rm.start();
MockNM nm1 = rm.registerNode(DEFAULT_HOST + ":" + DEFAULT_PORT, 6 * GB);
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(rm, 6);
Review Comment:
Same as my comments from before: A getter shouldn't do too much besides
getting the object. If cs is not used please create a separate init method with
the desired params and call that. If someone checks this code and sees that the
cs variable is not used he/she might simply remove it, but then the tests start
to fail. This is quite misleading.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestApplicationACLs.java:
##########
@@ -155,6 +157,7 @@ public void run() {
UserGroupInformation.createUserForTesting(SUPER_USER,
new String[] { SUPER_GROUP });
resourceManager.start();
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(resourceManager, 6);
Review Comment:
Same misleading getter.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerTestUtilities.java:
##########
@@ -229,4 +243,48 @@ public static void checkNodeResourceUsage(int expected,
NodeManager node) {
Assert.assertEquals(expected, node.getUsed().getMemorySize());
node.checkResourceUsage();
}
+
+ public static CapacityScheduler getCapacityScheduler(MockRM rm, int
memoryGb) {
+ return getCapacityScheduler(rm, memoryGb, 16, Collections.emptyMap());
+ }
+
+ public static CapacityScheduler getCapacityScheduler(MockRM rm, int
memoryGb, int cores) {
+ return getCapacityScheduler(rm, memoryGb, cores, Collections.emptyMap());
+ }
+
+ public static CapacityScheduler getCapacityScheduler(
+ MockRM rm, int memoryGb, int cores, Map<String, String> nameToValues
+ ) {
+ if(!(rm.getResourceScheduler() instanceof CapacityScheduler)) {
+ return null;
+ }
+ NullRMNodeLabelsManager mgr = (NullRMNodeLabelsManager)
rm.getRMContext().getNodeLabelManager();
+ CapacitySchedulerQueueCapacityHandler queueController =
+ new CapacitySchedulerQueueCapacityHandler(mgr);
+ CapacityScheduler cs = (CapacityScheduler) rm.getResourceScheduler();
+ Map<String, Long> others = new HashMap<>(nameToValues.size());
+ for (Map.Entry<String, String> nameToValue : nameToValues.entrySet()) {
+ others.put(nameToValue.getKey(), Long.valueOf(nameToValue.getValue()));
+ }
+ Resource clusterResource = Resource.newInstance(memoryGb * GB, cores,
others);
+ mgr.setResourceForLabel(CommonNodeLabelsManager.NO_LABEL, clusterResource);
+ queueController.updateRoot(cs.getQueue("root"), clusterResource);
+ updateChildren(queueController, clusterResource, cs.getQueue("root"));
+ return cs;
+ }
+
+ public static void updateChildren(
+ CapacitySchedulerQueueCapacityHandler queueController,
+ Resource clusterResource,
+ CSQueue queue
+ ) {
+ queueController.updateChildren(clusterResource, queue);
+ List<CSQueue> childs = queue.getChildQueues();
Review Comment:
Typo: childs -> children
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestWorkPreservingRMRestart.java:
##########
@@ -316,7 +316,7 @@ private Configuration getSchedulerDynamicConfiguration()
throws IOException {
// 5. Check if all running containers are recovered,
// 6. Verify the scheduler state like attempt info,
// 7. Verify the queue/user metrics for the dynamic reservable queue.
- @Test(timeout = 30000)
+ @Test(timeout = 60000)
Review Comment:
Did this timeout increase fix the test? If yes this might be a symptom of a
code change.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAbsoluteResourceWithAutoQueue.java:
##########
@@ -170,8 +171,11 @@ public void testAutoCreateLeafQueueCreation() throws
Exception {
// Add few nodes
mockRM.registerNode("127.0.0.1:1234", 250 * GB, 40);
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(mockRM, 250,40);
+
setupGroupQueueMappings(QUEUED, cs.getConfiguration(), "%user");
cs.reinitialize(cs.getConfiguration(), mockRM.getRMContext());
+ cs = CapacitySchedulerTestUtilities.getCapacityScheduler(mockRM, 250,40);
Review Comment:
Just an idea: why not override the reinitialize with a custom method which
does the extra steps described in getCapacityScheduler?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAmbiguousLeafs.java:
##########
@@ -23,8 +23,11 @@
import org.apache.hadoop.yarn.conf.YarnConfiguration;
import org.apache.hadoop.yarn.exceptions.YarnException;
import org.apache.hadoop.yarn.server.resourcemanager.MockRM;
+import
org.apache.hadoop.yarn.server.resourcemanager.nodelabels.NullRMNodeLabelsManager;
import org.apache.hadoop.yarn.server.resourcemanager.rmapp.RMAppState;
import org.apache.hadoop.yarn.util.resource.Resources;
+
+import org.junit.Ignore;
Review Comment:
Nit: unused.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestLeafQueue.java:
##########
@@ -243,9 +246,14 @@ private void setUpInternal(ResourceCalculator rC, boolean
withNodeLabels)
ROOT,
queues, queues,
TestUtils.spyHook);
- mockCsQm.setRootQueue(root);
+ when(mockCsQm.getRootQueue()).thenReturn(root);
Review Comment:
What's the reason behind this change? What's changed in mockCSQM that
results in calling setRootQueue not being enough?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/ApplicationMasterServiceTestBase.java:
##########
@@ -508,6 +511,8 @@ public void testUpdateTrackingUrl() throws Exception {
// Register node1
MockNM nm1 = rm.registerNode(DEFAULT_HOST + ":" + DEFAULT_PORT, 6 * GB);
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(rm, 6);
Review Comment:
Same as above.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestLeafQueue.java:
##########
@@ -3715,7 +3804,7 @@ public void testLocalityDelaysAfterQueueRefresh() throws
Exception {
assertEquals(600, e.getRackLocalityAdditionalDelay());
}
- @Test (timeout = 30000)
+ @Test (timeout = 300000)
Review Comment:
Is this needed?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/MockRM.java:
##########
@@ -144,6 +144,9 @@ public MockRM(Configuration conf, RMStateStore store,
boolean useRealElector) {
this(conf, store, true, useRealElector);
}
+ public MockRM(ResourceManager resourceManager) {
+ this(resourceManager.getConfig(),
resourceManager.getRMContext().getStateStore(), true, false);
Review Comment:
Nit: Should we actually create a MockRM from an RM? Why not call the
appropriate MockRM constructor instead of the normal RM?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerAutoQueueCreation.java:
##########
@@ -878,8 +885,8 @@ public void
testClusterResourceUpdationOnAutoCreatedLeafQueues() throws
// unregister one NM.
newMockRM.unRegisterNode(nm3);
- Resource MIN_RES_UPDATED = Resources.createResource(12800, 2);
- Resource MAX_RES_UPDATED = Resources.createResource(128000, 20);
+ Resource MIN_RES_UPDATED = Resources.createResource(14438, 6);
+ Resource MAX_RES_UPDATED = Resources.createResource(144384, 68);
Review Comment:
This impacts asserts, why was this changed?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCapacitySchedulerMaxParallelApps.java:
##########
@@ -40,6 +40,7 @@
import org.apache.hadoop.yarn.server.resourcemanager.MockRMAppSubmissionData;
import org.apache.hadoop.yarn.server.resourcemanager.MockRMAppSubmitter;
import org.apache.hadoop.yarn.server.resourcemanager.RMContext;
+import
org.apache.hadoop.yarn.server.resourcemanager.nodelabels.NullRMNodeLabelsManager;
Review Comment:
Nit: seems unused.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestClientRMService.java:
##########
@@ -646,6 +647,8 @@ public void testForceKillApplication() throws Exception {
rm.init(conf);
rm.start();
+ CapacitySchedulerTestUtilities.getCapacityScheduler(rm, 8,8);
Review Comment:
As above.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/RMHATestBase.java:
##########
@@ -218,5 +238,8 @@ protected void startRMs(MockRM rm1, Configuration
confForRM1, MockRM rm2,
rm1.adminService.transitionToActive(requestInfo);
Assert.assertTrue(rm1.getRMContext().getHAServiceState()
== HAServiceState.ACTIVE);
+ CapacitySchedulerTestUtilities.getCapacityScheduler(rm1, 8, 8);
+ CapacitySchedulerTestUtilities.getCapacityScheduler(rm1, 8, 8);
Review Comment:
Why call this twice for the same rm?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/rmapp/TestNodesListManager.java:
##########
@@ -65,6 +68,7 @@ protected Dispatcher createDispatcher() {
};
rm.start();
MockNM nm1 = rm.registerNode("h1:1234", 28000);
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(rm, 28);
Review Comment:
Nit: 28 GB and 28000 are not the same.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/RMHATestBase.java:
##########
@@ -126,6 +130,12 @@ protected Configuration loadNewConfiguration()
throws IOException, YarnException {
return confForRM1;
}
+
+ @Override
+ void refreshAll() throws ServiceFailedException {
+ super.refreshAll();
+ CapacitySchedulerTestUtilities.getCapacityScheduler((MockRM)
this.rm, 8, 8);
Review Comment:
Same as above, getCapacityScheduler is really misleading.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/TestContainerResourceUsage.java:
##########
@@ -77,6 +81,7 @@ public void testUsageWithOneAttemptAndOneContainer() throws
Exception {
MockNM nm =
new MockNM("127.0.0.1:1234", 15120, rm.getResourceTrackerService());
nm.registerNode();
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(rm, 16);
Review Comment:
16 seems like an arbirtary number. If it's needed can you please explain in
a comment?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAbsoluteResourceConfiguration.java:
##########
@@ -704,7 +704,7 @@ public void
testEffectiveResourceAfterReducingClusterResource()
rm.registerNode("127.0.0.2:1234", 125 * GB, 20);
// Get queue object to verify min/max resource configuration.
- CapacityScheduler cs = (CapacityScheduler) rm.getResourceScheduler();
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(rm, 250,40);
Review Comment:
AFAICS we only add one node with 125 GB and 20 vcores, where does the other
125 GB/20 vcore come from?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestCSAllocateCustomResource.java:
##########
@@ -47,6 +47,7 @@
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;
+import org.junit.Ignore;
Review Comment:
Nit: unused.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerTestUtilities.java:
##########
@@ -229,4 +243,48 @@ public static void checkNodeResourceUsage(int expected,
NodeManager node) {
Assert.assertEquals(expected, node.getUsed().getMemorySize());
node.checkResourceUsage();
}
+
+ public static CapacityScheduler getCapacityScheduler(MockRM rm, int
memoryGb) {
+ return getCapacityScheduler(rm, memoryGb, 16, Collections.emptyMap());
+ }
+
+ public static CapacityScheduler getCapacityScheduler(MockRM rm, int
memoryGb, int cores) {
+ return getCapacityScheduler(rm, memoryGb, cores, Collections.emptyMap());
+ }
+
+ public static CapacityScheduler getCapacityScheduler(
+ MockRM rm, int memoryGb, int cores, Map<String, String> nameToValues
Review Comment:
Can you rename nameToValues to a more descriptive name?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerTestUtilities.java:
##########
@@ -229,4 +243,48 @@ public static void checkNodeResourceUsage(int expected,
NodeManager node) {
Assert.assertEquals(expected, node.getUsed().getMemorySize());
node.checkResourceUsage();
}
+
+ public static CapacityScheduler getCapacityScheduler(MockRM rm, int
memoryGb) {
+ return getCapacityScheduler(rm, memoryGb, 16, Collections.emptyMap());
+ }
+
+ public static CapacityScheduler getCapacityScheduler(MockRM rm, int
memoryGb, int cores) {
+ return getCapacityScheduler(rm, memoryGb, cores, Collections.emptyMap());
+ }
+
+ public static CapacityScheduler getCapacityScheduler(
+ MockRM rm, int memoryGb, int cores, Map<String, String> nameToValues
+ ) {
+ if(!(rm.getResourceScheduler() instanceof CapacityScheduler)) {
+ return null;
+ }
Review Comment:
Is this actually needed? Is there a test which calls
CapacitySchedulerTestUtilities.getCapacityScheduler on a Fair/FifoScheduler?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestApplicationLimits.java:
##########
@@ -194,6 +207,8 @@ public void testAMResourceLimit() throws Exception {
Resource clusterResource = Resource.newInstance(80 * GB, 40);
root.updateClusterResource(clusterResource, new ResourceLimits(
clusterResource));
+ LeafQueue queue = (LeafQueue) root.getChildQueues().stream().filter(
+ child ->
child.getQueueName().equals(A)).findFirst().orElseThrow(NoSuchElementException::new);
Review Comment:
Why is the original queue object (which is a spy: `spy(new
LeafQueue(queueContext, A, root, null));`) hidden?
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/test/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/TestAbsoluteResourceWithAutoQueue.java:
##########
@@ -170,8 +171,11 @@ public void testAutoCreateLeafQueueCreation() throws
Exception {
// Add few nodes
mockRM.registerNode("127.0.0.1:1234", 250 * GB, 40);
+ CapacityScheduler cs =
CapacitySchedulerTestUtilities.getCapacityScheduler(mockRM, 250,40);
Review Comment:
cs is already an inherited variable, and you hide it with the local one.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
