brumi1024 commented on code in PR #6148:
URL: https://github.com/apache/hadoop/pull/6148#discussion_r1364065198
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/AbstractParentQueue.java:
##########
@@ -490,6 +490,12 @@ public CSQueue createNewQueue(String childQueuePath,
boolean isLeaf)
String queueShortName = childQueuePath.substring(
childQueuePath.lastIndexOf(".") + 1);
+ if (StringUtils.isEmpty(queueShortName)) {
+ throw new SchedulerDynamicEditException(
+ "Trying to create new queue=" + childQueuePath
+ + ", which has empty leaf shortname.");
+ }
Review Comment:
Not sure about this. Going up the callchain:
1. `AbstractParentQueue.createNewQueue` is only called from
`ParentQueue.addDynamicChildQueue`,
2. which (through another method) is called from
`CapacitySchedulerQueueManager.createAutoQueue`,
3. which is called from `CapacitySchedulerQueueManager.createQueue`, where
my second comment should apply
There is one difference: upon determining whether we're using legacy auto
creation or the flexible one we're using different methods, and the flexible
auto queue creation (method: `CapacitySchedulerQueueManager.createQueue
.createAutoQueue(queue)`) manipulates the QueuePath using `List<String>
parentsToCreate = determineMissingParents(queue)`. If an empty string part can
be added to the list, or can slip through I think it's probably there, which I
think should be a better place for these checks.
##########
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager/src/main/java/org/apache/hadoop/yarn/server/resourcemanager/scheduler/capacity/CapacitySchedulerQueueManager.java:
##########
@@ -509,6 +509,12 @@ public AbstractLeafQueue createQueue(QueuePath queue)
String leafQueueName = queue.getLeafName();
String parentQueueName = queue.getParent();
+ if (StringUtils.isEmpty(leafQueueName)) {
+ throw new SchedulerDynamicEditException(
+ "Trying to create new queue=" + queue.getFullPath()
+ + ", which has empty leaf shortname.");
+ }
+
Review Comment:
I don't think this part is necessary. QueueManager.createQueue(QueuePath) is
only called from CapacityScheduler.getOrCreateQueueFromPlacementContext (at
least from production code), and it has the following check right before this
call:
```
//we need to make sure there are no empty path parts present
if (queuePath.hasEmptyPart()) {
LOG.error("Application submitted to invalid path due to empty parts: " +
"'{}'", queuePath);
return null;
}
```
So essentially we should not be able to reach this from production code,
only from tests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: common-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: common-issues-h...@hadoop.apache.org
