[
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<String, Set<String>> 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<String, Integer> 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<String, Set<String>> 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<String, Integer> 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.
> 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, YARN-9879.POC001.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
