[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17051481#comment-17051481
]
Szilard Nemeth commented on YARN-9879:
--------------------------------------
Hi [~shuzirra] ,
Phew... This was pretty hard to review, spent at least 2 hours with it.
Thanks for working on this patch, good job, this change is incredible. :)
*In general, +1 for your approach by introducing CSQueueStore.*
*MY MAIN COMMENTS:*
1. I can see many TODOs in the patch. Do you want to address them in the next
patch?
Make sure you remove all the TODOs you have added to the code as they could
not be part of the commit.
2. Make sure to adhere to the 80 chars line limit: I saw some very long
comments.
3. I think I spot something suspicious with
CapacitySchedulerPreemptionContext:
The reader methods (getQueuePartition, getQueuePartitions) are using the full
queue path, like you are using it in
FifoIntraQueuePreemptionPlugin#skipContainerBasedOnIntraQueuePolicy
However, the implementation of this interface is passing a simple queue name
in ProportionalCapacityPreemptionPolicy#addPartitionToUnderServedQueues. Can
you please double check your changes around this code?
4. In QueuePath#QueuePath(java.lang.String): The value of leafQueue is assigne
to two fields: leafQueue, fullPath. Could you please add a comment here? I
don't get what's going on by just reading the code.
5. Please add javadoc to methods: CapacityScheduler#normalizeQueueName and
CapacityScheduler#isAmbiguous
6. TestCSQueueStore: I guess you will add something more in here :)
*COMMENTS FOR CSQueueStore:*
1. Can you please add a javadoc to the class CSQueueStore, to its fields and
to its main methods (at least the publicly accessible ones +
addShortNameMapping)?
2. Method getFullNameQueues can be package-private.
3. Method getShortNameQueues can be package-private.
4. There's an unnecessary comment in this class:
{code:java}
// shortNameToFullName.entrySet().stream().forEach(e ->
System.out.println("<>" + e));
// return null;
{code}
5. Comment could be a javadoc instead:
{code:java}
//we must synchronize here because we need to maintain multiple maps to be
//in sync, and concurrent hashMap does not help with that
{code}
6. Unnecessary commented code in method add
7. In method remove, you have an unnecessary containsKey check, intellij
reports this as well:
{code:java}
if (shortNameToFullName.containsKey(shortName)) {
shortNameToFullName.remove(shortName);
}
{code}
Remove will remove the mapping if it does exist, otherwise it won't do anything.
I would remove the containsKey check, unless you explicitly want to highlight
it.
8. Can you please add curly braces to the if in remove(java.lang.String)?
9. Method getQueueCount is unused
10. Method getByFullName can be package-private
11. Method getByShortName can be package-private
12. Method isAmbiguous can be package-private
13. In method getByShortName, can you add curly braces to the if?
*RENAMINGS:*
1. I found many occurrences of:
{code:java}
queueName = queue.getQueuePath
{code}
and
{code:java}
String leafQueueName = leafQueue.getQueuePath();
{code}
throughout your patch.
Please rename ALL the variables to queuePath (or maybe fullQueueName) as it's
pretty confusing like this.
2. Please rename the first parameter of
GuaranteedOrZeroCapacityOverTimePolicy.LeafQueueState#addLeafQueueStateIfNotExists
to queuePath as this method now receives a queue path instead of a name of the
leaf queue.
3. You have a call in
FifoIntraQueuePreemptionPlugin#skipContainerBasedOnIntraQueuePolicy:
{code:java}
TempQueuePerPartition tq = context.getQueueByPartition(queueName, partition);
{code}
I think it'd be a good idea to rename the parameter of
CapacitySchedulerPreemptionContext#getQueueByPartition to fullQueueName or add
a short javadoc to this method.
4. Please rename the 'queueName' parameter to 'fullQueueName' in
CapacityScheduler#checkAndGetApplicationPriority
5. Please rename the 'leafQueueName' parameter to 'fullQueueName' in
QueuePlacementRuleUtils#validateQueueMappingUnderParentQueue.
6. Please rename the parameter "queueName" to "fullQueueName" in methods
checkAbsoluteCapacity / checkMaxCapacity CSQueueUtils#checkMaxCapacity
7. Please rename local variable called "leafQueueName" to "fullQueueName" in
CapacityScheduler#markContainerForKillable.
8. Please rename local variable called "leafQueueName" to "fullQueueName" in
CapacityScheduler#markContainerForNonKillable.
*NITS:*
1. There's an unused import in QueuePlacementRuleUtils
2. I can see some whitespace only changes in
CapacitySchedulerConfigValidator#validateQueueHierarchy. Please remove them
from the patch if they are not necessary.
3. CapacitySchedulerQueueManager#normalizeQueueName(String name) could be
private.
4. CapacitySchedulerQueueManager#getQueueByShortName is unused.
5. Parameters of CapacitySchedulerConfigValidator#validateQueueHierarchy look
very weird.
> Allow multiple leaf queues with the same name in CS
> ---------------------------------------------------
>
> Key: YARN-9879
> URL: https://issues.apache.org/jira/browse/YARN-9879
> Project: Hadoop YARN
> Issue Type: Sub-task
> Reporter: Gergely Pollak
> Assignee: Gergely Pollak
> Priority: Major
> Labels: fs2cs
> Attachments: CSQueue.getQueueUsage.txt, DesignDoc_v1.pdf,
> YARN-9879.POC001.patch, YARN-9879.POC002.patch, YARN-9879.POC003.patch,
> YARN-9879.POC004.patch, YARN-9879.POC005.patch, YARN-9879.POC006.patch,
> YARN-9879.POC007.patch, YARN-9879.POC008.patch, YARN-9879.POC009.patch
>
>
> Currently the leaf queue's name must be unique regardless of its position in
> the queue hierarchy.
> Design doc and first proposal is being made, I'll attach it as soon as it's
> done.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
---------------------------------------------------------------------
To unsubscribe, e-mail: yarn-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: yarn-issues-h...@hadoop.apache.org
