9uapaw commented on a change in pull request #3550:
URL: https://github.com/apache/hadoop/pull/3550#discussion_r740835594
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/monitor/capacity/ProportionalCapacityPreemptionPolicy.java
##########
@@ -791,11 +791,7 @@ public Resource getPartitionResource(String partition) {
public void addPartitionToUnderServedQueues(String queueName,
String partition) {
LinkedHashSet<String> underServedQueues = partitionToUnderServedQueues
- .get(partition);
- if (null == underServedQueues) {
- underServedQueues = new LinkedHashSet<String>();
- partitionToUnderServedQueues.put(partition, underServedQueues);
- }
+ .computeIfAbsent(partition, k -> new LinkedHashSet<>());
Review comment:
Nit: I think the complaint of IntelliJ is invalid here, and
LinkedHashSet<String>::new is allowed here instead of the lambda.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -152,4 +159,8 @@ protected Resource getReservationQueueResourceIfExists(Plan
plan,
return reservationResource;
}
+ @Override
+ protected String getReservationIdFromQueueName(String resQueueName) {
+ return resQueueName.substring(resQueueName.lastIndexOf(".") + 1);
Review comment:
I think queueName is more like a queuePath here. Also, the QueuePath
class has getLeafName method, which is probably what you want 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/QueueNodeLabelsSettings.java
##########
@@ -31,26 +31,25 @@
public class QueueNodeLabelsSettings {
private final CSQueue parent;
private final String queuePath;
- private final CapacitySchedulerContext csContext;
private Set<String> accessibleLabels;
private Set<String> configuredNodeLabels;
private String defaultLabelExpression;
public QueueNodeLabelsSettings(CapacitySchedulerConfiguration configuration,
CSQueue parent,
String queuePath,
- CapacitySchedulerContext csContext) throws IOException {
+ ConfiguredNodeLabels configuredNodeLabelsParam) throws IOException {
Review comment:
Nit: I think just ignore the checkstyle warning and name this as
configuredNodeLabels.
##########
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
##########
@@ -40,14 +42,14 @@ public CSQueuePreemptionSettings(
* NOTE: Cross-queue preemptability is inherited from a queue's parent.
*
* @param q queue to check preemption state
- * @param csContext
* @param configuration capacity scheduler config
* @return true if queue has cross-queue preemption disabled, false otherwise
*/
private boolean isQueueHierarchyPreemptionDisabled(CSQueue q,
- CapacitySchedulerContext csContext, CapacitySchedulerConfiguration
configuration) {
+ CapacitySchedulerConfiguration configuration,
+ CapacitySchedulerConfiguration originalSchedulerConfiguration) {
Review comment:
I have failed to reason about the concept of configuration vs
originalSchedulerConfiguration. Is it truly necessary to have both of them? If
yes, please add some documentation in which you explain its necessity.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -114,8 +116,13 @@ protected void createDefaultReservationQueue(
PlanQueue planQueue = (PlanQueue)queue;
if (cs.getQueue(defReservationId) == null) {
try {
+ CSAutoCreatedLeafQueueProperties queueProperties = new
CSAutoCreatedLeafQueueProperties(
+ planQueue.getQueuePath() + "." + defReservationId,
+ cs.getMinimumResourceCapability());
+
ReservationQueue defQueue =
- new ReservationQueue(cs, defReservationId, planQueue);
+ new ReservationQueue(cs.getCapacitySchedulerQueueManager(),
+ queueProperties, planQueue);
cs.addQueue(defQueue);
} catch (SchedulerDynamicEditException e) {
LOG.warn(
Review comment:
This block shares some similarities with the block in
addReservationQueue. It could be extracted.
##########
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/ReservationQueue.java
##########
@@ -37,9 +37,17 @@
private PlanQueue parent;
- public ReservationQueue(CapacitySchedulerContext cs, String queueName,
+ public ReservationQueue(CapacitySchedulerQueueManager queueManager,
+ CSAutoCreatedLeafQueueProperties autoCreatedLeafQueueProperties,
PlanQueue parent) throws IOException {
- super(cs, queueName, parent, null);
+ super(queueManager, autoCreatedLeafQueueProperties, parent, null);
+ super.setupQueueConfigs(queueManager.getClusterResource(),
+ queueManager.getSchedulerConfiguration());
+
+ LOG.debug("Initialized ReservationQueue: name={}, fullname={}",
Review comment:
This is a recurring idiom, could be extracted to the super class and use
the name of the concrete class there.
##########
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/TestAbsoluteResourceConfiguration.java
##########
@@ -211,7 +211,10 @@ public void
testSimpleMinMaxResourceConfigurartionPerQueue()
CapacityScheduler cs = (CapacityScheduler) rm.getResourceScheduler();
ManagedParentQueue parentQueue = (ManagedParentQueue) cs.getQueue(QUEUED);
- AutoCreatedLeafQueue d1 = new AutoCreatedLeafQueue(cs, "d1", parentQueue);
+ CSAutoCreatedLeafQueueProperties queuePropertiesD1 = new
CSAutoCreatedLeafQueueProperties(
+ parentQueue.getQueuePath() + ".d1", cs.getMinimumAllocation());
Review comment:
Please use the new QueuePath class here.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -94,14 +95,15 @@ protected void addReservationQueue(
String planQueueName, Queue queue, String currResId) {
PlanQueue planQueue = (PlanQueue)queue;
try {
+ CSAutoCreatedLeafQueueProperties autoCreatedLeafQueueProperties =
+ new CSAutoCreatedLeafQueueProperties(planQueue.getQueuePath() + "."
Review comment:
Using the createNewLeaf method of the new QueuePath class would be
better here, as it is the same logic.
##########
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
##########
@@ -119,24 +120,32 @@ public void setUp() throws IOException {
when(csContext.getResourceCalculator()).
thenReturn(resourceCalculator);
when(csContext.getRMContext()).thenReturn(rmContext);
- when(csContext.getPreemptionManager()).thenReturn(new PreemptionManager());
-
+ when(csContext.getPreemptionManager()).thenReturn(preemptionManager);
+
+ CapacitySchedulerQueueManager queueManager = new
CapacitySchedulerQueueManager(csContext,
+ rmContext.getNodeLabelManager(), preemptionManager, null,
+ null);
+
when(csContext.getCapacitySchedulerQueueManager()).thenReturn(queueManager);
+
RMContainerTokenSecretManager containerTokenSecretManager =
new RMContainerTokenSecretManager(conf);
containerTokenSecretManager.rollMasterKey();
when(csContext.getContainerTokenSecretManager()).thenReturn(
containerTokenSecretManager);
CSQueueStore queues = new CSQueueStore();
- root = CapacitySchedulerQueueManager
- .parseQueue(csContext, csConf, null, "root",
- queues, queues,
- TestUtils.spyHook);
+ root = queueManager.parseQueue(null, "root",
+ queues, queues, TestUtils.spyHook);
+ queueManager.setRootQueue(root);
root.updateClusterResource(clusterResource,
new ResourceLimits(clusterResource));
- queue = spy(new LeafQueue(csContext, A, root, null));
- QueueResourceQuotas queueResourceQuotas = ((LeafQueue) queues.get(A))
+ CSLeafQueueProperties csQueuePropertiesA =
+ new CSLeafQueueProperties(root.getQueuePath() + "." + A,
Review comment:
See previous comment.
##########
File path:
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/reservation/CapacitySchedulerPlanFollower.java
##########
@@ -114,8 +116,13 @@ protected void createDefaultReservationQueue(
PlanQueue planQueue = (PlanQueue)queue;
if (cs.getQueue(defReservationId) == null) {
try {
+ CSAutoCreatedLeafQueueProperties queueProperties = new
CSAutoCreatedLeafQueueProperties(
+ planQueue.getQueuePath() + "." + defReservationId,
Review comment:
See comment before.
--
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]
