[
https://issues.apache.org/jira/browse/YARN-11079?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17718566#comment-17718566
]
ASF GitHub Bot commented on YARN-11079:
---------------------------------------
susheel-gupta commented on PR #5380:
URL: https://github.com/apache/hadoop/pull/5380#issuecomment-1531437238
> Hi @susheel-gupta , Just took a look at your changes in this PR.
>
> I compared the old version of ParentQueue vs. AbstractParentQueue and
found some differences:
>
> * the private int field with name 'numApplications' was volatile, now it
is changed to AtomicInteger. What is the reason of this?
> * In the method called 'addApplication', the writelock was locked during
incrementing numApplications, now I can see it removed. Do you know why this
was necessary?
> * Same for method called 'removeApplication'
>
> Otherwise LGTM
@szilard-nemeth There was this
[warning](https://ci-hadoop.apache.org/job/hadoop-multibranch/job/PR-5380/7/artifact/out/new-spotbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.html)
where they asked to fix the spotbugs, so I added this
[commit](https://github.com/apache/hadoop/pull/5380/commits/0f0d8f21823c23d49baa620a62111e7c3a269193)
.
With the above changes in this
[commit](https://github.com/apache/hadoop/pull/5380/commits/0f0d8f21823c23d49baa620a62111e7c3a269193)
, the numApplications field is an instance of AtomicInteger, which provides
an atomic incrementAndGet method to increment the value of the counter
atomically. Since AtomicInteger provides atomic operations, we don't need to
use locks or synchronization to ensure atomicity.
With these changes, the "VO_VOLATILE_INCREMENT: An increment to a volatile
field isn't atomic" issue should be resolved, and the code should now increment
the numApplications count atomically.
References:
- https://www.digitalocean.com/community/tutorials/atomicinteger-java
- https://www.baeldung.com/java-atomic-variables
> Make an AbstractParentQueue to store common ParentQueue and
> ManagedParentQueue functionality
> --------------------------------------------------------------------------------------------
>
> Key: YARN-11079
> URL: https://issues.apache.org/jira/browse/YARN-11079
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: capacity scheduler
> Reporter: Benjamin Teke
> Assignee: Susheel Gupta
> Priority: Major
> Labels: pull-request-available
>
> ParentQueue is an instantiable class which stores the necessary functionality
> of parent queues, however it is also extended by the
> AbstractManagedParentQueue, which is an abstract class for storing managed
> parent queue functionality. Since legacy AQC doesn't allow dynamic queues
> next to static ones, managed parent queues technically behave like leaf
> queues by not having any static child queues when created. This structure and
> behaviour is really error prone, as for example if someone is not completely
> aware of this and simply changes the checking order by first checking if the
> queue in question is a ParentQueue in a method like
> MappingRuleValidationContextImpl.isDynamicParent can result a completely
> wrong return value (as a ManagedParent is a dynamic parent, but currently
> it's also a ParentQueue, and ManagedParent cannot have the
> isEligibleForAutoQueueCreation as true, so the method will return false).
> {code:java}
> private boolean isDynamicParent(CSQueue queue) {
> if (queue == null) {
> return false;
> }
> if (queue instanceof ManagedParentQueue) {
> return true;
> }
> if (queue instanceof ParentQueue) {
> return ((ParentQueue)queue).isEligibleForAutoQueueCreation();
> }
> return false;
> }
> {code}
> Similarly to YARN-11024 an AbstractParentQueue class should be created to
> completely separate the managed parents from the instantiable ParentQueue
> class.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
