9uapaw commented on a change in pull request #3550:
URL: https://github.com/apache/hadoop/pull/3550#discussion_r763811301
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CSQueuePreemptionSettings.java
##########
@@ -60,7 +62,7 @@ private boolean isQueueHierarchyPreemptionDisabled(CSQueue q,
// on, then q does not have preemption disabled (default=false, below)
// unless the preemption_disabled property is explicitly set.
if (parentQ == null) {
- return configuration.getPreemptionDisabled(q.getQueuePath(), false);
+ return
originalSchedulerConfiguration.getPreemptionDisabled(q.getQueuePath(), false);
Review comment:
originalSchedulerConfiguration is inteded to replace the
csContext.getConfiguration. Is it intentional, that you replace the
configuration with originalSchedulerConfiguration here?
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ManagedParentQueue.java
##########
@@ -134,23 +134,24 @@ public void reinitialize(CSQueue newlyParsedQueue,
Resource clusterResource)
private void initializeQueueManagementPolicy() throws IOException {
queueManagementPolicy =
- csContext.getConfiguration().getAutoCreatedQueueManagementPolicyClass(
+
queueContext.getConfiguration().getAutoCreatedQueueManagementPolicyClass(
getQueuePath());
- queueManagementPolicy.init(csContext, this);
+ queueManagementPolicy.init(null, this);
Review comment:
I sincerely doubt, that anyone has ever created a custom
QueueManagementPolicy. That being said, we can not rely on assumptions, still I
think the right step here would be to modify the interface, because it was not
annotated as Stable. My vote here is to remove the csContext parameter.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/UsersManager.java
##########
@@ -835,7 +829,7 @@ Resource computeUserLimit(String userName, Resource
clusterResource,
+ ", Partition=" + nodePartition
+ ", resourceUsed=" + resourceUsed
+ ", maxUserLimit=" + maxUserLimit
- + ", userWeight=" + weight
+ + ", userWeight=" + getUser(userName).getWeight()
Review comment:
Accessing the weight was fixed in YARN-10996, please do not revert this
change.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/ParentQueue.java
##########
@@ -139,13 +131,12 @@ private ParentQueue(CapacitySchedulerContext cs,
this.childQueues = new ArrayList<>();
this.allowZeroCapacitySum =
- cs.getConfiguration().getAllowZeroCapacitySum(getQueuePath());
+ queueContext.getConfiguration()
+ .getAllowZeroCapacitySum(getQueuePath());
- setupQueueConfigs(cs.getClusterResource(), csConf);
+ setupQueueConfigs(queueContext.getClusterResource(),
queueContext.getConfiguration());
- LOG.info("Initialized parent-queue " + queueName +
- " name=" + queueName +
- ", fullname=" + getQueuePath());
+ LOG.debug("Initialized ParentQueue: name={}, fullname={}", queueName,
getQueuePath());
Review comment:
This is a recurring pattern, which could be generalized in
AbstractCSQueue instead.
##########
File path:
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
##########
@@ -288,18 +294,26 @@ public void testLimitsComputation() throws Exception {
when(csContext.getMaximumResourceCapability()).
thenReturn(Resources.createResource(16*GB, 16));
when(csContext.getResourceCalculator()).thenReturn(resourceCalculator);
+ when(csContext.getPreemptionManager()).thenReturn(new PreemptionManager());
when(csContext.getRMContext()).thenReturn(rmContext);
when(csContext.getPreemptionManager()).thenReturn(new PreemptionManager());
-
+
+ CapacitySchedulerQueueManager queueManager = new
CapacitySchedulerQueueManager(conf,
+ rmContext.getNodeLabelManager(), null);
+
when(csContext.getCapacitySchedulerQueueManager()).thenReturn(queueManager);
+
// Say cluster has 100 nodes of 16G each
Resource clusterResource =
Resources.createResource(100 * 16 * GB, 100 * 16);
when(csContext.getClusterResource()).thenReturn(clusterResource);
+ CapacitySchedulerQueueContext queueContext = new
CapacitySchedulerQueueContext(csContext);
+
Review comment:
This snippet is repeated multiple times. It could be extracted and
reused.
--
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]
