[ 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<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, 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<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 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. > 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