[ 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