[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[ https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17060899#comment-17060899 ] Gergely Pollak edited comment on YARN-9879 at 3/17/20, 1:09 PM: In the latest patch, there are mostly cosmetic changes. I've removed most of the TODOs as [~wangda] and [~snemeth] suggested, will create follow up Jira based on those. Fixed additional checkstyle issues, and added a proper error message when a queue is ambiguously referenced during submission, so the user will now get feedback, that the queue exists, but actually multiple queues exist with the same name. Also dropped the POC prefix, since it's getting near completion, so the latest patch is YARN-9879.014.patch was (Author: shuzirra): In the latest patch, there are mostly cosmetic changes. I've removed most of the TODOs as [~wangda] and [~snemeth] suggested, will create follow up Jira based on those. Fixed additional checkstyle issues, and added a proper error message when a queue is ambiguously referenced during submission, so the user will now get feedback, that the queue exists, but actually multiple queues exist with the same name. > 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.014.patch, 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, YARN-9879.POC010.patch, YARN-9879.POC011.patch, > YARN-9879.POC012.patch, YARN-9879.POC013.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
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17057274#comment-17057274
]
Wangda Tan edited comment on YARN-9879 at 3/11/20, 5:55 PM:
Thanks [~shuzirra] for uploading another monster patch!
I didn't check other places, I only checked the CSQueueStore related items:
*Nits: *
- CapacitySchedulerQueueManager#getShortNameQueues please mark @VisibleByTests
- Similarily, mark CSQueueStore#getShortNameQueues @VisibleByTests
*Primary: Locking of the class still have many issues: *
For all methods will be accessed by external class. Make sure that:
1) Avoid using synchronized lock when read/write lock present.
2) ALL external read-only methods protected by readlock.
3) ALL external writable methods protected by writelock.
4) Use
{code}
try {
lock.(read/or write).lock()
.. your logic ..
} catch (exception) {
// if there's any
} finally {
lock.(read/or write).unlock()
}
{code}
To make sure lock is always released: Example: CapacityScheduler#serviceStop
5) After the above changes, you can remove all usage of {{ConcurrentHashMap}},
it is bad for performance with locks. Hashmap will be way faster under the
protection of lock.
was (Author: wangda):
Thanks [~shuzirra] for uploading another monster patch!
I didn't check other places, I only checked the CSQueueStore related items:
*Nits: *
- CapacitySchedulerQueueManager#getShortNameQueues please mark @VisibleByTests
- Similarily, mark CSQueueStore#getShortNameQueues @VisibleByTests
*Primary: Locking of the class still have many issues: *
For all methods will be accessed by external class. Make sure that:
1) There're no synchronized lock.
2) All external read-only method use readlock.
3) All external writable method use writelock.
4) Use
{code}
try {
lock.(read/or write).lock()
.. your logic ..
} catch (exception) {
// if there's any
} finally {
lock.(read/or write).unlock()
}
{code}
To make sure lock is always released: Example: CapacityScheduler#serviceStop
5) After above changes, you can remove all usage of {{ConcurrentHashMap}}, it
is bad for performance with locks. Hashmap will be way faster under protection
of lock.
> 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,
> YARN-9879.POC010.patch, YARN-9879.POC011.patch, YARN-9879.POC012.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
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17022718#comment-17022718
]
Peter Bacsko edited comment on YARN-9879 at 1/24/20 12:42 PM:
--
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Three things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, v -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public synchronized void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue instanceof LeafQueue &&
leafCount.computeIfPresent(shortName, (k,v) -> v - 1) == 0) {
shortNameToFullName.remove(shortName);
}
}
{noformat}
#3 In {{get()}} is important to check ambiguous mappings, so an exception must
be thrown if leafCount > 1.
was (Author: pbacsko):
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Three things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, v -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue i
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17022718#comment-17022718
]
Peter Bacsko edited comment on YARN-9879 at 1/24/20 6:33 AM:
-
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Three things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, v -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue instanceof LeafQueue &&
leafCount.computeIfPresent(shortName, (k,v) -> v - 1) == 0) {
shortNameToFullName.remove(shortName);
}
}
{noformat}
#3 In {{get()}} is important to check ambiguous mappings, so an exception must
be thrown if leafCount > 1.
was (Author: pbacsko):
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Three things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, k -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue instanceof LeafQ
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17022718#comment-17022718
]
Peter Bacsko edited comment on YARN-9879 at 1/24/20 6:32 AM:
-
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Three things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, k -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue instanceof LeafQueue &&
leafCount.computeIfPresent(shortName, (k,v) -> v - 1) == 0) {
shortNameToFullName.remove(shortName);
}
}
{noformat}
#3 In {{get()}} is important to check ambiguous mappings, so an exception must
be thrown if leafCount > 1.
was (Author: pbacsko):
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Three things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's of how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, k -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue instanceof Le
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17022718#comment-17022718
]
Peter Bacsko edited comment on YARN-9879 at 1/24/20 6:22 AM:
-
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Three things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's of how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, k -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue instanceof LeafQueue &&
leafCount.computeIfPresent(shortName, (k,v) -> v - 1) == 0) {
shortNameToFullName.remove(shortName);
}
}
{noformat}
#3 In {{get()}} is important to check ambiguous mappings, so an exception must
be thrown if leafCount > 1.
was (Author: pbacsko):
Hey folks, I can see that a lengthy conversation is already going on, but I'll
try to keep my one short.
Regarding {{getQueueName()}} / {{getQueuePath()}}, it's up to you to decide, I
don't have enough context right now.
I'm trying to be constructive from code readability standpoint.
Two things that stand out to me are the following:
#1
{{private final Map> ambiguousShortNames = new
HashMap<>();}}
My question to [~shuzirra] is: do we need to keep track of what queues a short
name is mapped to? Do we use this information anywhere? Because if we use it as
a counter, then it's simply much easier to have a
{{private final Map leafCount = new HashMap<>();}}
And quite obviously you don't have ambiguity if leafCount == 1.
Because of this, the {{addShortNameMapping()}} is already a bit hard to grasp.
#2 I would synchronize the public method {{add()}}, not the private method.
To show what I was thinking of, here's of how I'd code add/remove:
{noformat}
// Keep as it as
public synchronized void add(CSQueue queue) {
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
fullNameQueues.put(fullName, queue);
if (queue instanceof LeafQueue) {
addShortNameMapping(shortName, fullName);
}
}
private void addShortNameMapping(String shortName, String fullName) {
// initialize if necessary
leafCount.computeIfAbsent(shortName, k -> 0);
if (leafCount.computeIfPresent(shortName, (k,v) -> v + 1) > 1) {
LOG.warn("Multiple mapping for queue {}!", shortName);
} else {
shortNameToFullName.put(shortName, fullName);
}
}
public void remove(CSQueue queue) {
//if no queue is specified, we can consider it already removed, also
consistent
//with hashmap behaviour, so no new issues will be caused by it
if (queue == null) {
return;
}
String fullName = queue.getQueueName();
String shortName = queue.getQueueShortName();
//removing from the full and short name maps as well
fullNameQueues.remove(fullName);
if (queue instanceof Le
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[ https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17021280#comment-17021280 ] Gergely Pollak edited comment on YARN-9879 at 1/22/20 6:10 PM: --- Thank you for your feedback [~leftnoteasy] and [~wilfreds]. Originally I tried to keep the getQueueName's behavior, but as I started to investigate it's behavior I've realized we MUST change the way it works. First let's start with a simple question: What is the purpose of the queue's name? Why does it have one, what do we want to use it for? (Ok these are actually 3 questions) As I see in the code the queue name's main purpose is to IDENTIFY a queue, and not just some nice display string. This means the name MUST identify uniquely the queue. Queues are looked up by their name, hence it must be unique or all those references can break. So this is the reason I changed it's behavior to return a unique identifier (the queue's path). Obviously I must check if it breaks anything, and fix it, but allowing multiple leaf queues with the same name is inherently a breaking change. I just try to minimize the impact to change the reference internally to full name everywhere (as you both suggested earlier). About the API breaking. If we have an API which provides us with a queue name, and currently it is a short name, then anyone who uses it to reference to the queue by the provided name will fail in the case of name duplicates. If we return the full name of the queue, then it will still work for them, unless they build on the fact it is just a short name. As long as the queue name is used for queue identification, and not for string operations, it shouldn't cause any problem. Other cases must be identified. This is why I ended up with this approach. This way we change the queue naming once and for all to use full names, and we adjust services which would fail on this change. But we cannot keep the short queue name as reference and have multiple queues with the same name, it's just impossible. This patch will already introduce some changes which can cause issues in already working systems and it might be better to do all invasive changes at once. I can use the getQueuePath (almost) everywhere where we currently using getQueueName, but the result would be the same, with some severe inconsistencies: Using short names would result you being able to get the name of a queue, but you wouldn't be able to get your queue by that very same name from the queue manager. This is just confusing, inconsistent, and not maintenable in my opinion. The quemanager.get(queue.getQueueName()) call can result in NULL or error! (when the queue name is not unique) This is not good practice in my opinion. We need the ambiguous queue list, because we provide a remove method, which can result in a previously ambiguous name becoming ambiguous, and it's much faster to get it from a hashmap O(1), and then check the size of the Set O(1), instead of looking through all queues to see if the collision have been resolved O( n ). The short name map has been introduced for the very same reason, when we look up a queue, we just look it up in 2 HashMaps 2 x O(1), instead of iterating through all queue names and splicing the last part for short name O( n ). So all in all, I've sacrificed some memory space for a drastic speed increase. O( n ) vs O(1) might not seem a huge improvement in the case of a few queues, but considering the queue parse method will make a get call for each queue to check if it is already present in the store, we have a complexity of O(n*n), which IS something to think about. Please help me to think this through one more time with taking my reasons into consideration, thank you. was (Author: shuzirra): Thank you for your feedback [~leftnoteasy] and [~wilfreds]. Originally I tried to keep the getQueueName's behavior, but as I started to investigate it's behavior I've realized we MUST change the way it works. First let's start with a simple question: What is the purpose of the queue's name? Why does it have one, what do we want to use it for? (Ok these are actually 3 questions) As I see in the code the queue name's main purpose is to IDENTIFY a queue, and not just some nice display string. This means the name MUST identify uniquely the queue. Queues are looked up by their name, hence it must be unique or all those references can break. So this is the reason I changed it's behavior to return a unique identifier (the queue's path). Obviously I must check if it breaks anything, and fix it, but allowing multiple leaf queues with the same name is inherently a breaking change. I just try to minimize the impact to change the reference internally to full name everywhere (as you both suggested earlier). About the API breaking. If we have an API which provides us with a queue name, and
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[ https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17021280#comment-17021280 ] Gergely Pollak edited comment on YARN-9879 at 1/22/20 5:01 PM: --- Thank you for your feedback [~leftnoteasy] and [~wilfreds]. Originally I tried to keep the getQueueName's behavior, but as I started to investigate it's behavior I've realized we MUST change the way it works. First let's start with a simple question: What is the purpose of the queue's name? Why does it have one, what do we want to use it for? (Ok these are actually 3 questions) As I see in the code the queue name's main purpose is to IDENTIFY a queue, and not just some nice display string. This means the name MUST identify uniquely the queue. Queues are looked up by their name, hence it must be unique or all those references can break. So this is the reason I changed it's behavior to return a unique identifier (the queue's path). Obviously I must check if it breaks anything, and fix it, but allowing multiple leaf queues with the same name is inherently a breaking change. I just try to minimize the impact to change the reference internally to full name everywhere (as you both suggested earlier). About the API breaking. If we have an API which provides us with a queue name, and currently it is a short name, then anyone who uses it to reference to the queue by the provided name will fail in the case of name duplicates. If we return the full name of the queue, then it will still work for them, unless they build on the fact it is just a short name. As long as the queue name is used for queue identification, and not for string operations, it shouldn't cause any problem. Other cases must be identified. This is why I ended up with this approach. This way we change the queue naming once and for all to use full names, and we adjust services which would fail on this change. But we cannot keep the short queue name as reference and have multiple queues with the same name, it's just impossible. This patch will already introduce some changes which can cause issues in already working systems and it might be better to do all invasive changes at once. I can use the getQueuePath (almost) everywhere where we currently using getQueueName, but the result would be the same, with some severe inconsistencies: Using short names would result you being able to get the name of a queue, but you wouldn't be able to get your queue by that very same name from the queue manager. This is just confusing, inconsistent, and not maintenable in my opinion. The quemanager.get(queue.getQueueName()) call can result in NULL or error! (when the queue name is not unique) This is not good practice in my opinion. We need the ambiguous queue list, because we provide a remove method, which can result in a previously ambiguous name becoming ambiguous, and it's much faster to get it from a hashmap O(1), and then check the size of the Set O(1), instead of looking through all queues to see if the collision have been resolved O(n). The short name map has been introduced for the very same reason, when we look up a queue, we just look it up in 2 HashMaps 2 x O(1), instead of iterating through all queue names and splicing the last part for short name O(n). So all in all, I've sacrificed some memory space for a drastic speed increase. O(n) vs O(1) might not seem a huge improvement in the case of a few queues, but considering the queue parse method will make a get call for each queue to check if it is already present in the store, we have a complexity of O(n*n), which IS something to think about. Please help me to think this through one more time with taking my reasons into consideration, thank you. was (Author: shuzirra): Thank you for your feedback Wilfred Spiegelenburg and Wangda Tan. Originally I tried to keep the getQueueName's behavior, but as I started to investigate it's behavior I've realized we MUST change the way it works. First let's start with a simple question: What is the purpose of the queue's name? Why does it have one, what do we want to use it for? (Ok these are actually 3 questions) As I see in the code the queue name's main purpose is to IDENTIFY a queue, and not just some nice display string. This means the name MUST identify uniquely the queue. Queues are looked up by their name, hence it must be unique or all those references can break. So this is the reason I changed it's behavior to return a unique identifier (the queue's path). Obviously I must check if it breaks anything, and fix it, but allowing multiple leaf queues with the same name is inherently a breaking change. I just try to minimize the impact to change the reference internally to full name everywhere (as you both suggested earlier). About the API breaking. If we have an API which provides us with a queue name, and
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[ https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17015404#comment-17015404 ] Wangda Tan edited comment on YARN-9879 at 1/14/20 10:01 PM: [~snemeth], most of the explanation looks reasonable to me. Regarding how to prevent breaking existing CS queue contract. Instead of adding a flag to each queue, I suggest to have a global config in CS about allow duplicated leaf queue name or not. Why I'm opposed to add the flag to each queue? To me, use a queue name, or queue path is an intuitive choice of a user (not admin). If the queue name had duplicates, it should fail and give you the right reason. If everybody think we should not implicitly change the CS behavior to allow duplicate-named leaf queues, a top-level CS config should be sufficient (like ...duplicated-queue-names.allowed), and clearly document it may cause existing app failures. This won't add any burden for user to understand, and it is also relatively easy for admin to understand. Anything config added to the queue hierarchy seems a bit tricky. (Like admin has to think about how is the queue override looks like, etc.). And for the auto-created queue case it is not obvious to add such configs, etc.My big lesson learned is we should add as less knobs as we could, too many knobs will increase our support areas a lot and make code hard to be maintained. was (Author: leftnoteasy): [~snemeth], most of the explanation looks reasonable to me. Regarding how to prevent breaking existing CS queue contract. Instead of adding a flag to each queue, I suggest to have a global config in CS about allow duplicated leaf queue name or not. Why I'm opposed to add the flag to each queue? To me, use a queue name, or queue path is an intuitive choice of a user (not admin). If the queue name had duplicates, it should fail and give you the right reason. If everybody think we should not implicitly change the CS behavior to allow duplicate-named leaf queues, a top-level CS config should be sufficient (like ...duplicated-queue-names.allowed), and clearly document it may cause existing app failures. This won't add any burden for user to understand, and it is also relatively easy for admin to understand. Anything config added to the queue hierarchy seems a bit tricky. > 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 > Attachments: DesignDoc_v1.pdf > > > 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
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17013171#comment-17013171
]
Szilard Nemeth edited comment on YARN-9879 at 1/10/20 8:04 PM:
---
Thank you for your feedbacks [~wilfreds], [~pbacsko], [~leftnoteasy] and thans
[~shuzirra] for moving this forward!
Chiming in a bit late, but I would like to give my opinion as well. Apologies
for being so lengthy, but I wanted to quote and reply some parts of the
comments had been given so far.
Please note that some parts of what I'm saying here is taking the FS to CS
migration into account.
*1. Quoting from Wilfred's comment:*
A.
{quote}If we simply relax the rule that the leaf queue must be unique in the
system in favour of the fact that a queue must be unique based on the full
queue path. This does not break existing configurations as the unique leaf
queue is also unique when you take into account the whole path. That means
there is nothing for the current clusters that needs to change. Internally the
scheduler does have to change to make sure that all references use the queue
path. This will require a lot of changes throughout the scheduler when you look
up a queue and the way we store the reference if it is not directly to the leaf
queue.
{quote}
I do agree with this, existing configurations does not break. However, allowing
non-unique leaf queues can make the CS behave weirdly and inconsistently as
queues are being added later on, as [~shuzirra] described this lately in his
comment. More on this matter later.
B.
{quote}When an application is submitted with just a queue name (not a path) we
expect that the name is a unique leaf queue name. If that queue does not exist
or is not uniquely identifiable we reject the application submission.
Resolution of the real leaf queue follows the same steps as it does now. The
queue name in the end is converted to the correct leaf queue identified by the
a path. For existing configurations nothing has changed. Internally we hide all
the changes.
{quote}
AND
C.
{quote}The important part is applying a new configuration. If the configuration
adds a leaf queue that is not unique the configuration update currently is
rejected. With this change we would allow that config to become active. This
could break existing applications when they try to submit to the leaf queue
that is no longer unique.
We should at least log and warn clearly in the response of the update. Maybe
even show it in the UI or we could ask for a confirmation. The first update
that adds a non unique queue to the configuration should always fail
complaining loudly. It should then keep warning the user and rejecting the
update unless a confirmation flag is set to force the update through. After the
first update that would not be needed anymore.
Reading a config from a file or store which is used to initialise the
scheduler should not trigger such behaviour. We still should show a warning in
the logs to make sure it is not lost.
{quote}
About what you said: "This could break existing applications when they try to
submit to the leaf queue that is no longer unique."
Again, this is really problematic and error-prone, as detailed by [~shuzirra]
in his comment above.
We discussed this together with Gergo and I'm trying to give more weight on
how tricky this case can get and what I'm doing now is just reiterate on
Gergo's comment
Imagine the following queue setup:
{code:java}
root
|a
|
|b
||b
||c
||d
|
|c
|e
|f
{code}
If someone is adding root.c.b, queue 'b' is non-unique anymore without even
knowing or taking care of root.b.b is the other queue 'b'.
Even if we log and warn in the response of the update, adding the root.c.b
queue can break the app submitted to queue root.b.b.
I can imagine many real-world scenarios where users were submitting apps to a
queue (root.b.b) with just the leaf queue name ('b') and their app submission
would get suddenly rejected since another queue do exist with the same name.
Just consider a user-group mapping and two same usernames exist under different
organizations/user groups and we can be in the situation what I depicted. I
also agree to Gergo that we can't expect or assume every user and apps
submission is guarded by monitoring or warning systems.
I think it's definitely not a good idea to break existing users and app
submissions with this change.
*2. Quoting from Wangda's comment:*
{quote}I personally think it is not a big deal if application reject reasons
from RM can clearly guide users to use full qualified queue path when
duplicated queue names exists. It is like if a team has only one Peter we can
use the first name only otherwise we will add last name to avoid confusion. It
isn't counter-intuitive to me.
{quote}
How can we know it
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17013171#comment-17013171
]
Szilard Nemeth edited comment on YARN-9879 at 1/10/20 8:04 PM:
---
Thank you for your feedbacks [~wilfreds], [~pbacsko], [~leftnoteasy] and thans
[~shuzirra] for moving this forward!
Chiming in a bit late, but I would like to give my opinion as well. Apologies
for being so lengthy, but I wanted to quote and reply some parts of the
comments had been given so far.
Please note that some parts of what I'm saying here is taking the FS to CS
migration into account.
1. Quoting from Wilfred's comment:
A.
{quote}If we simply relax the rule that the leaf queue must be unique in the
system in favour of the fact that a queue must be unique based on the full
queue path. This does not break existing configurations as the unique leaf
queue is also unique when you take into account the whole path. That means
there is nothing for the current clusters that needs to change. Internally the
scheduler does have to change to make sure that all references use the queue
path. This will require a lot of changes throughout the scheduler when you look
up a queue and the way we store the reference if it is not directly to the leaf
queue.
{quote}
I do agree with this, existing configurations does not break. However,
allowing non-unique leaf queues can make the CS behave weirdly and
inconsistently as queues are being added later on, as [~shuzirra] described
this lately in his comment. More on this matter later.
B.
{quote}When an application is submitted with just a queue name (not a path) we
expect that the name is a unique leaf queue name. If that queue does not exist
or is not uniquely identifiable we reject the application submission.
Resolution of the real leaf queue follows the same steps as it does now. The
queue name in the end is converted to the correct leaf queue identified by the
a path. For existing configurations nothing has changed. Internally we hide all
the changes.
{quote}
AND
C.
{quote}The important part is applying a new configuration. If the configuration
adds a leaf queue that is not unique the configuration update currently is
rejected. With this change we would allow that config to become active. This
could break existing applications when they try to submit to the leaf queue
that is no longer unique.
We should at least log and warn clearly in the response of the update. Maybe
even show it in the UI or we could ask for a confirmation. The first update
that adds a non unique queue to the configuration should always fail
complaining loudly. It should then keep warning the user and rejecting the
update unless a confirmation flag is set to force the update through. After the
first update that would not be needed anymore.
Reading a config from a file or store which is used to initialise the
scheduler should not trigger such behaviour. We still should show a warning in
the logs to make sure it is not lost.
{quote}
About what you said: "This could break existing applications when they try to
submit to the leaf queue that is no longer unique."
Again, this is really problematic and error-prone, as detailed by [~shuzirra]
in his comment above.
We discussed this together with Gergo and I'm trying to give more weight on
how tricky this case can get and what I'm doing now is just reiterate on
Gergo's comment
Imagine the following queue setup:
{code:java}
root
|a
|
|b
||b
||c
||d
|
|c
|e
|f
{code}
If someone is adding root.c.b, queue 'b' is non-unique anymore without even
knowing or taking care of root.b.b is the other queue 'b'.
Even if we log and warn in the response of the update, adding the root.c.b
queue can break the app submitted to queue root.b.b.
I can imagine many real-world scenarios where users were submitting apps to a
queue (root.b.b) with just the leaf queue name ('b') and their app submission
would get suddenly rejected since another queue do exist with the same name.
Just consider a user-group mapping and two same usernames exist under different
organizations/user groups and we can be in the situation what I depicted. I
also agree to Gergo that we can't expect or assume every user and apps
submission is guarded by monitoring or warning systems.
I think it's definitely not a good idea to break existing users and app
submissions with this change.
2. Quoting from Wangda's comment:
{quote}I personally think it is not a big deal if application reject reasons
from RM can clearly guide users to use full qualified queue path when
duplicated queue names exists. It is like if a team has only one Peter we can
use the first name only otherwise we will add last name to avoid confusion. It
isn't counter-intuitive to me.
{quote}
How can we know it's
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17009797#comment-17009797
]
Peter Bacsko edited comment on YARN-9879 at 1/7/20 2:55 PM:
[~wilfreds] based on your suggestion, here's what I came up with:
We can still maintain the HashMap with queueName->CSQueue, however we'd use two
levels:
1. Leaf queue -> full path
2. Full path -> CSQueue object
We additionally need an extra map which tells whether a leaf queue is unique.
So after some thinking, this is the semi-pseudocode that could possibly do the
job:
{noformat}
Map fullPathQueues;
Map leafToFullPath;
Map leafUnique;
public CSQueue getQueue(String queueName) {
if (fullPathName(queueName)) {
return fullPathQueues.get(queueName);
} else {
if (leafUnique.get(queueName)) {
String fullName = leafToFullPath.get(queueName);
return fullPathQueues.get(fullName);
} else {
throw new YarnException(queueName + " is not unique");
}
}
}
{noformat}
Obviously methods like {{addQueue()}}, {{removeQueue()}} should be updated too.
was (Author: pbacsko):
[~wilfreds] based on your suggestion, here's what I came up with:
We can still maintain the HashMap with queueName->CSQueue, however we'd use two
levels:
1. Leaf queue -> full path
2. Full path -> CSQueue object
We additionally need an extra map which tells whether a leaf queue is unique.
So after some thinking, this is the semi-pseudocode that could possibly do the
job:
{noformat}
Map fullPathQueues;
Map leafToFullPath;
Map leafUnique;
public CSQueue getQueue(String queueName) {
if (fullPathName(queueName)) {
return queues.get(queueName);
} else {
if (leafUnique.get(queueName)) {
String fullName = leafToFullPath.get(queueName);
return queues.get(fullName);
} else {
throw new YarnException(queueName + " is not unique");
}
}
}
{noformat}
Obviously methods like {{addQueue()}}, {{removeQueue()}} should be updated too.
> 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
> Attachments: DesignDoc_v1.pdf
>
>
> 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
[jira] [Comment Edited] (YARN-9879) Allow multiple leaf queues with the same name in CS
[
https://issues.apache.org/jira/browse/YARN-9879?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17009797#comment-17009797
]
Peter Bacsko edited comment on YARN-9879 at 1/7/20 2:54 PM:
[~wilfreds] based on your suggestion, here's what I came up with:
We can still maintain the HashMap with queueName->CSQueue, however we'd use two
levels:
1. Leaf queue -> full path
2. Full path -> CSQueue object
We additionally need an extra map which tells whether a leaf queue is unique.
So after some thinking, this is the semi-pseudocode that could possibly do the
job:
{noformat}
Map fullPathQueues;
Map leafToFullPath;
Map leafUnique;
public CSQueue getQueue(String queueName) {
if (fullPathName(queueName)) {
return queues.get(queueName);
} else {
if (leafUnique.get(queueName)) {
String fullName = leafToFullPath.get(queueName);
return queues.get(fullName);
} else {
throw new YarnException(queueName + " is not unique");
}
}
}
{noformat}
Obviously methods like {{addQueue()}}, {{removeQueue()}} should be updated too.
was (Author: pbacsko):
[~wilfreds] based on your suggestion, here's what I came up with:
We can still maintain the HashMap with queueName->CSQueue, however we'd use two
levels:
1. Leaf queue -> full path
2. Full path -> CSQueue object
We additionally need an extra map which tells whether a leaf queue is unique.
So after some thinking, this is the semi-pseudocode that could possibly do the
job:
{noformat}
MapfullPathQueues;
Map leafToFullPath;
Map leafUnique;
public CSQueue getQueue(String queueName) {
if (fullPathName(queueName)) {
return queues.get(queueName);
} else {
if (leafUnique.get(queueName)) {
String fullName = leafToFullPath.get(queueName);
return queues.get(fullName);
} else {
throw new YarnException(queueName + " is not unique");
}
}
}
{noformat}
Obviously methods like {{addQueue()}}, {{removeQueue()}} should be updated too.
> 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
> Attachments: DesignDoc_v1.pdf
>
>
> 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
