szilard-nemeth commented on a change in pull request #3430: URL: https://github.com/apache/hadoop/pull/3430#discussion_r711735992
########## 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/AbstractCSQueueUsageTracker.java ########## @@ -0,0 +1,78 @@ +/** + * 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; + +import org.apache.hadoop.yarn.server.resourcemanager.scheduler.QueueResourceQuotas; +import org.apache.hadoop.yarn.server.resourcemanager.scheduler.ResourceUsage; + +public class AbstractCSQueueUsageTracker { + private final CSQueueMetrics metrics; + private volatile int numContainers; + + /** + * The timestamp of the last submitted application to this queue. + * Only applies to dynamic queues. + */ + private long lastSubmittedTimestamp; + + /** + * Tracks resource usage by label like used-resource / pending-resource. + */ + private volatile ResourceUsage queueUsage; + + private final QueueResourceQuotas queueResourceQuotas; + + public AbstractCSQueueUsageTracker(CSQueueMetrics metrics) { + this.metrics = metrics; + this.queueUsage = new ResourceUsage(); + this.queueResourceQuotas = new QueueResourceQuotas(); + } + + public int getNumContainers() { + return numContainers; + } + + public synchronized void increaseNumContainers() { + numContainers++; + } + + public synchronized void decreaseNumContainers() { Review comment: Thanks for your comment @9uapaw. Let me share my opinion. Originally, numContainers was a volatile field of AbstractCSQueue and I just moved here. The thing is, it may have not been necessarily a volatile field in the first place as the getter of it was invoked by these methods: CapacitySchedulerLeafQueueInfo.CapacitySchedulerLeafQueueInfo(CapacityScheduler, LeafQueue) (org.apache.hadoop.yarn.server.resourcemanager.webapp.dao) ClusterMetricsInfo.ClusterMetricsInfo(ResourceScheduler) (org.apache.hadoop.yarn.server.resourcemanager.webapp.dao) LeafQueue.toString() (org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity) ParentQueue.toString() (org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity) The question of whether marking this field with the volatile keyword made sense really depends on if these methods are called from multiple threads or not. Internally, AbstractCSQueue increased and decreased the value with the ++/-- operators and IntelliJ showed a warning already, as it's not an atomic operation to use these operators on a volatile field. Volatile prevents all the threads from reading a non up to date value. Anyway, what you described makes sense for me so that we should use the queue-level lock, it's already happening as calls of increaseNumContainers / decreaseNumContainers as the respective callers are allocateResource / releaseResource that are making sure of guarding the usageTracker operations with the writeLock so I admit it doesn't really make sense to mark the methods synchronized. One possible caveat is that the increaseNumContainers / decreaseNumContainers methods are now protected, so it's not guaranteed at all that the only callee would be originated from AbstractCSQueue's methods. Alternatively, the writeLock could be passed to the CSQueueUsageTracker but I wouldn't prefer doing that. All in all, I think we should keep the volatile and keep the locks as is in AbstractCSQueue, but we will have the warning in IntelliJ as a consequence. -- 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]
