[
https://issues.apache.org/jira/browse/YARN-11041?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Tibor Kovács updated YARN-11041:
--------------------------------
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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-fde6885144b59bb06b2c3358780388d958829b13f68aceee7bb6d394bb5e0548]
|[~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] [ |https://github.com/9uapaw] [~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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
|[~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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
|[~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|https://github.com/apache/hadoop/pull/3660/files/0c3dd17c936260fc9c386dcabc6368b54b27aa82..39f4ec203377244f840e4593aa02386ff51cc3c4#diff-0adf8192c51cbe4671324f06f7f8cbd48898df0376bbcc516451a3bdb2b48d3b]
|[~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.|
was:
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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-fde6885144b59bb06b2c3358780388d958829b13f68aceee7bb6d394bb5e0548]
[~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] [ |https://github.com/9uapaw] [~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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
[~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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
[~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.
> Replace all occurences of queuePath with the new QueuePath class - followup
> ---------------------------------------------------------------------------
>
> Key: YARN-11041
> URL: https://issues.apache.org/jira/browse/YARN-11041
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: capacity scheduler
> Reporter: Tibor Kovács
> Assignee: Szilard Nemeth
> Priority: Major
> Labels: pull-request-available
>
> 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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-fde6885144b59bb06b2c3358780388d958829b13f68aceee7bb6d394bb5e0548]
>
> |[~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] [ |https://github.com/9uapaw] [~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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
>
> |[~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|https://github.com/apache/hadoop/pull/3660/files/f956918bc154d0e35fce07c5dd8be804eb007acc#diff-c4b0c5e70208f1e3cfbd5a86ffa2393e5c996cc8b45605d9d41abcb7e0bd382a]
> |[~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|https://github.com/apache/hadoop/pull/3660/files/0c3dd17c936260fc9c386dcabc6368b54b27aa82..39f4ec203377244f840e4593aa02386ff51cc3c4#diff-0adf8192c51cbe4671324f06f7f8cbd48898df0376bbcc516451a3bdb2b48d3b]
> |[~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.|
>
>
--
This message was sent by Atlassian Jira
(v8.20.1#820001)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
