[YARN-11041] Replace all occurences of queuePath with the new QueuePath class - followup

• Type: Sub-task
• Status: Resolved
• Priority: Major
• Resolution: Fixed
• Fix Version/s: 3.5.0
• Component/s: capacity scheduler
• Labels: pull-request-available
• Hadoop Flags: Reviewed
• Assignee: Peter Szucs
• Reporter: Tibor Kovács
• Created: 09/Dec/21 10:52
• Updated: 26/Jan/24 13:44
• Resolved: 26/Jan/24 13:44

Description

The QueuePath class was introduced in YARN-10897, however, its current adoption happened only for code changes after this JIRA. We need to adopt it retrospectively.

 

A lot of changes are introduced via ticket YARN-10982. The replacing should be continued by touching the next comments:

 

...g/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AutoCreatedQueueTemplate.java

snemeth https://github.com/apache/hadoop/pull/3660#discussion_r765012937 I think this could be also refactored in a follow-up jira so the string magic could probably be replaced with some more elegant solution. Though, I think this would be too much in this patch, hence I do suggest the follow-up jira.

snemeth https://github.com/apache/hadoop/pull/3660#discussion_r765013096 bteke   gandras [ |https://github.com/9uapaw] Thoughts?

bteke https://github.com/apache/hadoop/pull/3660#discussion_r765110750 +1, even the QueuePath object could have some kind of support for this.

gandras https://github.com/apache/hadoop/pull/3660#discussion_r765131244 Agreed, let's handle it in a followup!

 

---

 
...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java

snemeth https://github.com/apache/hadoop/pull/3660#discussion_r765023717 There are many string operations in this class: E.g. * getQueuePrefix that works with the full queue path getNodeLabelPrefix that also works with the full queue path

I suggest to create a static class, called "QueuePrefixes" or something like that and add some static methods there to convert the QueuePath object to those various queue prefix strings that are ultimately keys in the Configuration object.

 

---

 

...he/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerConfiguration.java

snemeth https://github.com/apache/hadoop/pull/3660#discussion_r765026119 This seems hacky, just based on the constructor parameter names of QueuePath: parent, leaf. The AQC Template prefix is not the leaf, obviously. Could we somehow circumvent this?

bteke https://github.com/apache/hadoop/pull/3660#discussion_r765126207 Maybe a factory method could be created, which returns a new QueuePath with the parent set as the original queuePath. I.e rootQueuePath.createChild(String childName) -> this could return a new QueuePath object with root.childName path, and rootQueuePath as parent.

snemeth https://github.com/apache/hadoop/pull/3660#discussion_r765039033 Looking at this getQueues method, I realized almost all the callers are using some kind of string magic that should be addressed with this patch. For example, take a look at: org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.conf.MutableCSConfigurationProvider#addQueue I think getQueues should also receive the QueuePath object instead of Strings.

 

---

 

.../src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CSQueue.java

bteke https://github.com/apache/hadoop/pull/3660#discussion_r765912967 Nit: Gets the queue path object. The object of the queue suggests a CSQueue object.

snemeth https://github.com/apache/hadoop/pull/3660#discussion_r765922133 Will fix the nit upon commit if I'm fine with the whole patch. Thanks for noticing.

 
 

Issue Links

• is a clone of: YARN-10982 Replace all occurences of queuePath with the new QueuePath class
• is related to: YARN-10983 Follow-up changes for YARN-10904
• links to: GitHub Pull Request #5332
• links to: GitHub Pull Request #6497
• links to: GitHub Pull Request #6499