[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266919#comment-17266919
]
Szilard Nemeth edited comment on YARN-10535 at 1/17/21, 9:03 PM:
-
Thanks [~shuzirra] for working on this,
Some comments:
*1. MappingRuleValidationHelper*
This code block could be extracted to a new method as it's being used from 2
different locations:
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, fullPath.split("\\."));
{code}
*Just looking this helper class from a higher-level, I think one way to make
this more readable and more easy to see through would be the following:*
1.1. Create a class that could parse a queue path, i.e. split it along dots and
store the resulted array into a field, can be called parts as you were calling
the local variable.
1.2. Add a method to this class that can get a specific element of the path.
This would just be a named method of returning an n-th element from the array
(0-based indexing, of course).
1.3. You may also add some helper methods like getFirst / getLast so the
indexing itself is hidden from the client code.
1.4. Another would be the one that can set/replace a queue path part with
something. This method would basically wrap this code:
{code:java}
parts.set(0, pathRootQueue.getQueuePath());
{code}
1.5. For operations like removing a particular path part, you can also have a
method that receives an index and removes the path part:
{code:java}
parts.remove(parts.size() - 1);
{code}
Similarly to 1.3., you can add a method that's called removeLast() that hides
the indexing.
1.6. For getting the parent path / grandparent path, you may also define a
getParentPath() method. I mean this code could be added to that method:
{code:java}
return parts.size() >= 1 ? String.join(".", parts) : "";
{code}
Other than that, MappingRuleValidationHelper looks good.
*2. MappingRuleValidationContextImpl:* Maybe I'm missing something but for me
it's very confusing that in MappingRuleValidationContextImpl, we have
validateDynamicQueuePath and validateStaticQueuePath methods.
As by checking the calls of validateStaticQueuePath plus looking at name of
it, this method is guaranteed to deal only with static queue paths.
Why do we have all the ValidationResult state handling for dynamically created
queues in this method, then?
*3. MappingRuleValidationContextImpl*
3.1 isDynamicParent could be a static method
3.2 Javadoc of "validateDynamicQueuePath" has a typo:
"@return true of the path is valid"
--> Should be: "@return true *if* the path is valid"
*3.3 Similarly to my comments for MappingRuleValidationHelper, I could imagine
a "more OOP approach" here as well.*
I would definitely create a small class to various operations.
The String iterator (called pointer) should be a field of the class.
Of course you may think the code will still be procedural (Mostly), I think
this would improve the readability a lot, especially from the perspective of
the clients of this new class:
3.3.1 Parse the queue path (split it into parts):
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, path.getFullPath().split("\\."));
{code}
3.3.2 Initial check for empty queue path should be in one separate method:
{code:java}
Iterator pointer = parts.iterator();
if (!pointer.hasNext()) {
//This should not happen since we only call validateDynamicQueuePath
//if we have found at least ONE dynamic part, which implies the path is
//not empty, so if we get here, I'm really curious what the path was,
//that's the reason we give back a theoretically "empty" path
throw new YarnException("Empty queue path provided '" + path + "'");
}
{code}
3.3.3 Checking the root with a dedicated method:
{code:java}
//If not even the root of the reference is static we cannot validate
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
{code}
3.3.4 Parsing the static part of the queue, storing it in a field of the class:
{code:java}
//getting the static part of the queue, we can only validate that
while (pointer.hasNext()) {
String nextPart = pointer.next();
if (isPathStatic(nextPart)) {
staticPartParent = staticPartBuffer.toString();
staticPartBuffer.append(DOT).append(nextPart);
} else {
//when we find the first dynamic part, we stop the search
break;
}
}
String staticPart = staticPartBuffer.toString();
{code}
3.3.5 Checking if the normalized static part is an instance of LeafQueue or not
could be also a dedicated method:
{code:java}
String normalizedStaticPart =
MappingRuleValidationHelper.normalizeQueuePathRoot(
queueManager, staticPart);
CSQueue queue = queueManager.getQueue(normalizedStaticPart);
//if the static part of our queue exists, and it's not a leaf queue,
//we cannot do any deeper validation
if (queue != null) {
if (queue instanceof LeafQueue) {
throw
[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266919#comment-17266919
]
Szilard Nemeth edited comment on YARN-10535 at 1/17/21, 9:02 PM:
-
Thanks [~shuzirra] for working on this,
Some comments:
*1. MappingRuleValidationHelper*
This code block could be extracted to a new method as it's being used from 2
different locations:
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, fullPath.split("\\."));
{code}
*Just looking this helper class from a higher-level, I think one way to make
this more readable and more easy to see through would be the following:*
1.1. Create a class that could parse a queue path, i.e. split it along dots and
store the resulted array into a field, can be called parts as you were calling
the local variable.
1.2. Add a method to this class that can get a specific element of the path.
This would just be a named method of returning an n-th element from the array
(0-based indexing, of course).
1.3. You may also add some helper methods like getFirst / getLast so the
indexing itself is hidden from the client code.
1.4. Another would be the one that can set/replace a queue path part with
something. This method would basically wrap this code:
{code:java}
parts.set(0, pathRootQueue.getQueuePath());
{code}
1.5. For operations like removing a particular path part, you can also have a
method that receives an index and removes the path part:
{code:java}
parts.remove(parts.size() - 1);
{code}
Similarly to 1.3., you can add a method that's called removeLast() that hides
the indexing.
1.6. For getting the parent path / grandparent path, you may also define a
getParentPath() method. I mean this code could be added to that method:
{code:java}
return parts.size() >= 1 ? String.join(".", parts) : "";
{code}
Other than that, MappingRuleValidationHelper looks good.
*2. MappingRuleValidationContextImpl:* Maybe I'm missing something but for me
it's very confusing that in MappingRuleValidationContextImpl, we have
validateDynamicQueuePath and validateStaticQueuePath methods.
As by checking the calls of validateStaticQueuePath plus looking at name of
it, this method is guaranteed to deal only with static queue paths.
Why do we have all the ValidationResult state handling for dynamically created
queues in this method, then?
*3. MappingRuleValidationContextImpl*
3.1 isDynamicParent could be a static method
3.2 Javadoc of "validateDynamicQueuePath" has a typo:
"@return true of the path is valid"
--> Should be: "@return true *if* the path is valid"
*3.3 Similarly to my comments for MappingRuleValidationHelper, I could imagine
a "more OOP approach" here as well.*
I would definitely create a small class to various operations.
The String iterator (called pointer) should be a field of the class.
Of course you may think the code will still be procedural (Mostly), I think
this would improve the readability a lot, especially from the perspective of
the clients of this new class:
3.3.1 Parse the queue path (split it into parts):
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, path.getFullPath().split("\\."));
{code}
3.3.2 Initial check for empty queue path should be in one separate method:
{code:java}
Iterator pointer = parts.iterator();
if (!pointer.hasNext()) {
//This should not happen since we only call validateDynamicQueuePath
//if we have found at least ONE dynamic part, which implies the path is
//not empty, so if we get here, I'm really curious what the path was,
//that's the reason we give back a theoretically "empty" path
throw new YarnException("Empty queue path provided '" + path + "'");
}
{code}
3.3.3 Checking the root with a dedicated method:
{code:java}
//If not even the root of the reference is static we cannot validate
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
{code}
3.3.4 Parsing the static part of the queue, storing it in a field of the class:
{code:java}
//getting the static part of the queue, we can only validate that
while (pointer.hasNext()) {
String nextPart = pointer.next();
if (isPathStatic(nextPart)) {
staticPartParent = staticPartBuffer.toString();
staticPartBuffer.append(DOT).append(nextPart);
} else {
//when we find the first dynamic part, we stop the search
break;
}
}
String staticPart = staticPartBuffer.toString();
{code}
3.3.5 Checking if the normalized static part is an instance of LeafQueue or not
could be also a dedicated method:
{code:java}
String normalizedStaticPart =
MappingRuleValidationHelper.normalizeQueuePathRoot(
queueManager, staticPart);
CSQueue queue = queueManager.getQueue(normalizedStaticPart);
//if the static part of our queue exists, and it's not a leaf queue,
//we cannot do any deeper validation
if (queue != null) {
if (queue instanceof LeafQueue) {
throw
[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266919#comment-17266919
]
Szilard Nemeth edited comment on YARN-10535 at 1/17/21, 9:02 PM:
-
Thanks [~shuzirra] for working on this,
Some comments:
*1. MappingRuleValidationHelper*
This code block could be extracted to a new method as it's being used from 2
different locations:
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, fullPath.split("\\."));
{code}
*Just looking this helper class from a higher-level, I think one way to make
this more readable and more easy to see through would be the following:*
1.1. Create a class that could parse a queue path, i.e. split it along dots and
store the resulted array into a field, can be called parts as you were calling
the local variable.
1.2. Add a method to this class that can get a specific element of the path.
This would just be a named method of returning an n-th element from the array
(0-based indexing, of course).
1.3. You may also add some helper methods like getFirst / getLast so the
indexing itself is hidden from the client code.
1.4. Another would be the one that can set/replace a queue path part with
something. This method would basically wrap this code:
{code:java}
parts.set(0, pathRootQueue.getQueuePath());
{code}
1.5. For operations like removing a particular path part, you can also have a
method that receives an index and removes the path part:
{code:java}
parts.remove(parts.size() - 1);
{code}
Similarly to 1.3., you can add a method that's called removeLast() that hides
the indexing.
1.6. For getting the parent path / grandparent path, you may also define a
getParentPath() method. I mean this code could be added to that method:
{code:java}
return parts.size() >= 1 ? String.join(".", parts) : "";
{code}
Other than that, MappingRuleValidationHelper looks good.
*2. MappingRuleValidationContextImpl:* Maybe I'm missing something but for me
it's very confusing that in MappingRuleValidationContextImpl, we have
validateDynamicQueuePath and validateStaticQueuePath methods.
As by checking the calls of validateStaticQueuePath plus looking at name of
it, this method is guaranteed to deal only with static queue paths.
Why do we have all the ValidationResult state handling for dynamically created
queues in this method, then?
*3. MappingRuleValidationContextImpl*
3.1 isDynamicParent could be a static method
3.2 Javadoc of "validateDynamicQueuePath" has a typo:
"@return true of the path is valid"
--> Should be: "@return true *if* the path is valid"
*3.3 Similarly to my comments for MappingRuleValidationHelper, I could imagine
a "more OOP approach" here as well.*
I would definitely create a small class to various operations.
The String iterator (called pointer) should be a field of the class.
Of course you may think the code will still be procuderul (Mostly), I think
this would improve the readability a lot, especially from the perspective of
the clients of this new class:
3.3.1 Parse the queue path (split it into parts):
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, path.getFullPath().split("\\."));
{code}
3.3.2 Initial check for empty queue path should be in one separate method:
{code:java}
Iterator pointer = parts.iterator();
if (!pointer.hasNext()) {
//This should not happen since we only call validateDynamicQueuePath
//if we have found at least ONE dynamic part, which implies the path is
//not empty, so if we get here, I'm really curious what the path was,
//that's the reason we give back a theoretically "empty" path
throw new YarnException("Empty queue path provided '" + path + "'");
}
{code}
3.3.3 Checking the root with a dedicated method:
{code:java}
//If not even the root of the reference is static we cannot validate
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
{code}
3.3.4 Parsing the static part of the queue, storing it in a field of the class:
{code:java}
//getting the static part of the queue, we can only validate that
while (pointer.hasNext()) {
String nextPart = pointer.next();
if (isPathStatic(nextPart)) {
staticPartParent = staticPartBuffer.toString();
staticPartBuffer.append(DOT).append(nextPart);
} else {
//when we find the first dynamic part, we stop the search
break;
}
}
String staticPart = staticPartBuffer.toString();
{code}
3.3.5 Checking if the normalized static part is an instance of LeafQueue or not
could be also a dedicated method:
{code:java}
String normalizedStaticPart =
MappingRuleValidationHelper.normalizeQueuePathRoot(
queueManager, staticPart);
CSQueue queue = queueManager.getQueue(normalizedStaticPart);
//if the static part of our queue exists, and it's not a leaf queue,
//we cannot do any deeper validation
if (queue != null) {
if (queue instanceof LeafQueue) {
throw
[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266919#comment-17266919
]
Szilard Nemeth edited comment on YARN-10535 at 1/17/21, 9:01 PM:
-
Thanks [~shuzirra] for working on this,
Some comments:
*1. MappingRuleValidationHelper*
This code block could be extracted to a new method as it's being used from 2
different locations:
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, fullPath.split("\\."));
{code}
*Just looking this helper class from a higher-level, I think one way to make
this more readable and more easy to see through would be the following:*
1.1. Create a class that could parse a queue path, i.e. split it along dots and
store the resulted array into a field, can be called parts as you were calling
the local variable.
1.2. Add a method to this class that can get a specific element of the path.
This would just be a named method of returning an n-th element from the array
(0-based indexing, of course).
1.3. You may also add some helper methods like getFirst / getLast so the
indexing itself is hidden from the client code.
1.4. Another would be the one that can set/replace a queue path part with
something. This method would basically wrap this code:
{code:java}
parts.set(0, pathRootQueue.getQueuePath());
{code}
1.5. For operations like removing a particular path part, you can also have a
method that receives an index and removes the path part:
{code:java}
parts.remove(parts.size() - 1);
{code}
Similarly to 1.3., you can add a method that's called removeLast() that hides
the indexing.
1.6. For getting the parent path / grandparent path, you may also define a
getParentPath() method. I mean this code could be added to that method:
{code:java}
return parts.size() >= 1 ? String.join(".", parts) : "";
{code}
Other than that, MappingRuleValidationHelper looks good.
*2. MappingRuleValidationContextImpl:* Maybe I'm missing something but for me
it's very confusing that in MappingRuleValidationContextImpl, we have
validateDynamicQueuePath and validateStaticQueuePath methods.
As by checking the calls of validateStaticQueuePath plus looking at name of
it, this method is guaranteed to deal only with static queue paths.
Why do we have all the ValidationResult state handling for dynamically created
queues in this method, then?
*3. MappingRuleValidationContextImpl*
3.1 isDynamicParent could be a static method
3.2 Javadoc of "validateDynamicQueuePath" has a typo:
"@return true of the path is valid"
--> Should be: "@return true *if* the path is valid"
3.3 Similarly to my comments for MappingRuleValidationHelper, I could imagine a
"more OOP approach" here as well.
3.3. I would definitely create a small class to various operations.
The String iterator (called pointer) should be a field of the class.
Of course you may think the code will still be procuderul (Mostly), I think
this would improve the readability a lot, especially from the perspective of
the clients of this new class:
3.3.1 Parse the queue path (split it into parts):
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, path.getFullPath().split("\\."));
{code}
3.3.2 Initial check for empty queue path should be in one separate method:
{code:java}
Iterator pointer = parts.iterator();
if (!pointer.hasNext()) {
//This should not happen since we only call validateDynamicQueuePath
//if we have found at least ONE dynamic part, which implies the path is
//not empty, so if we get here, I'm really curious what the path was,
//that's the reason we give back a theoretically "empty" path
throw new YarnException("Empty queue path provided '" + path + "'");
}
{code}
3.3.3 Checking the root with a dedicated method:
{code:java}
//If not even the root of the reference is static we cannot validate
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
{code}
3.3.4 Parsing the static part of the queue, storing it in a field of the class:
{code:java}
//getting the static part of the queue, we can only validate that
while (pointer.hasNext()) {
String nextPart = pointer.next();
if (isPathStatic(nextPart)) {
staticPartParent = staticPartBuffer.toString();
staticPartBuffer.append(DOT).append(nextPart);
} else {
//when we find the first dynamic part, we stop the search
break;
}
}
String staticPart = staticPartBuffer.toString();
{code}
3.3.5 Checking if the normalized static part is an instance of LeafQueue or not
could be also a dedicated method:
{code:java}
String normalizedStaticPart =
MappingRuleValidationHelper.normalizeQueuePathRoot(
queueManager, staticPart);
CSQueue queue = queueManager.getQueue(normalizedStaticPart);
//if the static part of our queue exists, and it's not a leaf queue,
//we cannot do any deeper validation
if (queue != null) {
if (queue instanceof LeafQueue) {
[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266919#comment-17266919
]
Szilard Nemeth edited comment on YARN-10535 at 1/17/21, 9:01 PM:
-
Thanks [~shuzirra] for working on this,
Some comments:
*1. MappingRuleValidationHelper*
This code block could be extracted to a new method as it's being used from 2
different locations:
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, fullPath.split("\\."));
{code}
*Just looking this helper class from a higher-level, I think one way to make
this more readable and more easy to see through would be the following:*
1.1. Create a class that could parse a queue path, i.e. split it along dots and
store the resulted array into a field, can be called parts as you were calling
the local variable.
1.2. Add a method to this class that can get a specific element of the path.
This would just be a named method of returning an n-th element from the array
(0-based indexing, of course).
1.3. You may also add some helper methods like getFirst / getLast so the
indexing itself is hidden from the client code.
1.4. Another would be the one that can set/replace a queue path part with
something. This method would basically wrap this code:
{code:java}
parts.set(0, pathRootQueue.getQueuePath());
{code}
1.5. For operations like removing a particular path part, you can also have a
method that receives an index and removes the path part:
{code:java}
parts.remove(parts.size() - 1);
{code}
Similarly to 1.3., you can add a method that's called removeLast() that hides
the indexing.
1.6. For getting the parent path / grandparent path, you may also define a
getParentPath() method. I mean this code could be added to that method:
{code:java}
return parts.size() >= 1 ? String.join(".", parts) : "";
{code}
Other than that, MappingRuleValidationHelper looks good.
*2. MappingRuleValidationContextImpl:* Maybe I'm missing something but for me
it's very confusing that in MappingRuleValidationContextImpl, we have
validateDynamicQueuePath and validateStaticQueuePath methods.
As by checking the calls of validateStaticQueuePath plus looking at name of
it, this method is guaranteed to deal only with static queue paths.
Why do we have all the ValidationResult state handling for dynamically created
queues in this method, then?
*3. MappingRuleValidationContextImpl*
3.1 isDynamicParent could be a static method
3.2 Javadoc of "validateDynamicQueuePath" has a typo:
"@return true of the path is valid"
--> Should be: "@return true *if* the path is valid"
*3.3 Similarly to my comments for MappingRuleValidationHelper, I could imagine
a "more OOP approach" here as well.*
3.3. I would definitely create a small class to various operations.
The String iterator (called pointer) should be a field of the class.
Of course you may think the code will still be procuderul (Mostly), I think
this would improve the readability a lot, especially from the perspective of
the clients of this new class:
3.3.1 Parse the queue path (split it into parts):
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, path.getFullPath().split("\\."));
{code}
3.3.2 Initial check for empty queue path should be in one separate method:
{code:java}
Iterator pointer = parts.iterator();
if (!pointer.hasNext()) {
//This should not happen since we only call validateDynamicQueuePath
//if we have found at least ONE dynamic part, which implies the path is
//not empty, so if we get here, I'm really curious what the path was,
//that's the reason we give back a theoretically "empty" path
throw new YarnException("Empty queue path provided '" + path + "'");
}
{code}
3.3.3 Checking the root with a dedicated method:
{code:java}
//If not even the root of the reference is static we cannot validate
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
{code}
3.3.4 Parsing the static part of the queue, storing it in a field of the class:
{code:java}
//getting the static part of the queue, we can only validate that
while (pointer.hasNext()) {
String nextPart = pointer.next();
if (isPathStatic(nextPart)) {
staticPartParent = staticPartBuffer.toString();
staticPartBuffer.append(DOT).append(nextPart);
} else {
//when we find the first dynamic part, we stop the search
break;
}
}
String staticPart = staticPartBuffer.toString();
{code}
3.3.5 Checking if the normalized static part is an instance of LeafQueue or not
could be also a dedicated method:
{code:java}
String normalizedStaticPart =
MappingRuleValidationHelper.normalizeQueuePathRoot(
queueManager, staticPart);
CSQueue queue = queueManager.getQueue(normalizedStaticPart);
//if the static part of our queue exists, and it's not a leaf queue,
//we cannot do any deeper validation
if (queue != null) {
if (queue instanceof LeafQueue) {
[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266919#comment-17266919
]
Szilard Nemeth edited comment on YARN-10535 at 1/17/21, 9:00 PM:
-
Thanks [~shuzirra] for working on this,
Some comments:
*1. MappingRuleValidationHelper*
This code block could be extracted to a new method as it's being used from 2
different locations:
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, fullPath.split("\\."));
{code}
*Just looking this helper class from a higher-level, I think one way to make
this more readable and more easy to see through would be the following:*
1.1. Create a class that could parse a queue path, i.e. split it along dots and
store the resulted array into a field, can be called parts as you were calling
the local variable.
1.2. Add a method to this class that can get a specific element of the path.
This would just be a named method of returning an n-th element from the array
(0-based indexing, of course).
1.3. You may also add some helper methods like getFirst / getLast so the
indexing itself is hidden from the client code.
1.4. Another would be the one that can set/replace a queue path part with
something. This method would basically wrap this code:
{code:java}
parts.set(0, pathRootQueue.getQueuePath());
{code}
1.5. For operations like removing a particular path part, you can also have a
method that receives an index and removes the path part:
{code:java}
parts.remove(parts.size() - 1);
{code}
Similarly to 1.3., you can add a method that's called removeLast() that hides
the indexing.
1.6. For getting the parent path / grandparent path, you may also define a
getParentPath() method. I mean this code could be added to that method:
{code:java}
return parts.size() >= 1 ? String.join(".", parts) : "";
{code}
Other than that, MappingRuleValidationHelper looks good.
*2. MappingRuleValidationContextImpl:* Maybe I'm missing something but for me
it's very confusing that in MappingRuleValidationContextImpl, we have
validateDynamicQueuePath and validateStaticQueuePath methods.
As by checking the calls of validateStaticQueuePath plus looking at name of
it, this method is guaranteed to deal only with static queue paths.
Why do we have all the ValidationResult state handling for dynamically created
queues in this method, then?
*3. MappingRuleValidationContextImpl*
3.1 isDynamicParent could be a static method
3.2 Javadoc of "validateDynamicQueuePath" has a typo:
"@return true of the path is valid"
--> Should be: "@return true if the path is valid"
3.3 Similarly to my comments for MappingRuleValidationHelper, I could imagine a
"more OOP approach" here as well.
3.3. I would definitely create a small class to various operations.
The String iterator (called pointer) should be a field of the class.
Of course you may think the code will still be procuderul (Mostly), I think
this would improve the readability a lot, especially from the perspective of
the clients of this new class:
3.3.1 Parse the queue path (split it into parts):
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, path.getFullPath().split("\\."));
{code}
3.3.2 Initial check for empty queue path should be in one separate method:
{code:java}
Iterator pointer = parts.iterator();
if (!pointer.hasNext()) {
//This should not happen since we only call validateDynamicQueuePath
//if we have found at least ONE dynamic part, which implies the path is
//not empty, so if we get here, I'm really curious what the path was,
//that's the reason we give back a theoretically "empty" path
throw new YarnException("Empty queue path provided '" + path + "'");
}
{code}
3.3.3 Checking the root with a dedicated method:
{code:java}
//If not even the root of the reference is static we cannot validate
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
{code}
3.3.4 Parsing the static part of the queue, storing it in a field of the class:
{code:java}
//getting the static part of the queue, we can only validate that
while (pointer.hasNext()) {
String nextPart = pointer.next();
if (isPathStatic(nextPart)) {
staticPartParent = staticPartBuffer.toString();
staticPartBuffer.append(DOT).append(nextPart);
} else {
//when we find the first dynamic part, we stop the search
break;
}
}
String staticPart = staticPartBuffer.toString();
{code}
3.3.5 Checking if the normalized static part is an instance of LeafQueue or not
could be also a dedicated method:
{code:java}
String normalizedStaticPart =
MappingRuleValidationHelper.normalizeQueuePathRoot(
queueManager, staticPart);
CSQueue queue = queueManager.getQueue(normalizedStaticPart);
//if the static part of our queue exists, and it's not a leaf queue,
//we cannot do any deeper validation
if (queue != null) {
if (queue instanceof LeafQueue) {
throw
[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266919#comment-17266919
]
Szilard Nemeth edited comment on YARN-10535 at 1/17/21, 8:46 PM:
-
Thanks [~shuzirra] for working on this,
Some comments:
*1. MappingRuleValidationHelper*
This code block could be extracted to a new method as it's being used from 2
different locations:
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, fullPath.split("\\."));
{code}
Just looking this helper class from a higher-level, I think one way to make
this more readable and more easy to see through would be the following:
1.1. Create a class that could parse a queue path, i.e. split it along dots and
store the resulted array into a field, can be called parts as you were calling
the local variable.
1.2. Add a method to this class that can get a specific element of the path.
This would just be a named method of returning an n-th element from the array
(0-based indexing, of course).
1.3. You may also add some helper methods like getFirst / getLast so the
indexing itself is hidden from the client code.
1.4. Another would be the one that can set/replace a queue path part with
something. This method would basically wrap this code:
{code:java}
parts.set(0, pathRootQueue.getQueuePath());
{code}
1.5. For operations like removing a particular path part, you can also have a
method that receives an index and removes the path part:
{code:java}
parts.remove(parts.size() - 1);
{code}
Similarly to 1.3., you can add a method that's called removeLast() that hides
the indexing.
1.6. For getting the parent path / grandparent path, you may also define a
getParentPath() method. I mean this code could be added to that method:
{code:java}
return parts.size() >= 1 ? String.join(".", parts) : "";
{code}
Other than that, MappingRuleValidationHelper looks good.
*2. MappingRuleValidationContextImpl:* Maybe I'm missing something but for me
it's very confusing that in MappingRuleValidationContextImpl, we have
validateDynamicQueuePath and validateStaticQueuePath methods.
As by checking the calls of validateStaticQueuePath plus looking at name of
it, this method is guaranteed to deal only with static queue paths.
Why do we have all the ValidationResult state handling for dynamically created
queues in this method, then?
*3. MappingRuleValidationContextImpl*
3.1 isDynamicParent could be a static method
3.2 Javadoc of "validateDynamicQueuePath" has a typo:
"@return true of the path is valid"
--> Should be: "@return true if the path is valid"
3.3 Similarly to my comments for MappingRuleValidationHelper, I could imagine a
"more OOP approach" here as well.
3.3. I would definitely create a small class to various operations.
The String iterator (called pointer) should be a field of the class.
Of course you may think the code will still be procuderul (Mostly), I think
this would improve the readability a lot, especially from the perspective of
the clients of this new class:
3.3.1 Parse the queue path (split it into parts):
{code:java}
ArrayList parts = new ArrayList<>();
Collections.addAll(parts, path.getFullPath().split("\\."));
{code}
3.3.2 Initial check for empty queue path should be in one separate method:
{code:java}
Iterator pointer = parts.iterator();
if (!pointer.hasNext()) {
//This should not happen since we only call validateDynamicQueuePath
//if we have found at least ONE dynamic part, which implies the path is
//not empty, so if we get here, I'm really curious what the path was,
//that's the reason we give back a theoretically "empty" path
throw new YarnException("Empty queue path provided '" + path + "'");
}
{code}
3.3.3 Checking the root with a dedicated method:
{code:java}
//If not even the root of the reference is static we cannot validate
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
{code}
3.3.4 Parsing the static part of the queue, storing it in a field of the class:
{code:java}
//getting the static part of the queue, we can only validate that
while (pointer.hasNext()) {
String nextPart = pointer.next();
if (isPathStatic(nextPart)) {
staticPartParent = staticPartBuffer.toString();
staticPartBuffer.append(DOT).append(nextPart);
} else {
//when we find the first dynamic part, we stop the search
break;
}
}
String staticPart = staticPartBuffer.toString();
{code}
3.3.5 Checking if the normalized static part is an instance of LeafQueue or not
could be also a dedicated method:
{code:java}
String normalizedStaticPart =
MappingRuleValidationHelper.normalizeQueuePathRoot(
queueManager, staticPart);
CSQueue queue = queueManager.getQueue(normalizedStaticPart);
//if the static part of our queue exists, and it's not a leaf queue,
//we cannot do any deeper validation
if (queue != null) {
if (queue instanceof LeafQueue) {
throw new
[jira] [Comment Edited] (YARN-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[ https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17266771#comment-17266771 ] Gergely Pollak edited comment on YARN-10535 at 1/17/21, 12:41 PM: -- Patchset#5 fixes the new checkstyle/findbugs/javadoc warnings introduced during patchet#4. Test failure is unrelated, test seems to be flaky. was (Author: shuzirra): Patchset#5 fixes the new checkstyle/findbugs/javadoc warnings introduced during patchet#4. > Make queue placement in CapacityScheduler compliant with auto-queue-placement > - > > Key: YARN-10535 > URL: https://issues.apache.org/jira/browse/YARN-10535 > Project: Hadoop YARN > Issue Type: Sub-task > Components: capacity scheduler >Reporter: Wangda Tan >Assignee: Gergely Pollak >Priority: Major > Attachments: YARN-10535.001.patch, YARN-10535.002.patch, > YARN-10535.003.patch, YARN-10535.004.patch, YARN-10535.005.patch > > > Once YARN-10506 is done, we need to call the API from the queue placement > policy to create queues. -- 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-10535) Make queue placement in CapacityScheduler compliant with auto-queue-placement
[
https://issues.apache.org/jira/browse/YARN-10535?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=17265947#comment-17265947
]
Peter Bacsko edited comment on YARN-10535 at 1/15/21, 11:41 AM:
Initial review:
1. Remove star imports: {{import java.util.*;}}
2. Remove star imports: {{import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.*;}}
3. Is this class used? {{import org.apache.hadoop.yarn.webapp.hamlet2.Hamlet;}}
4. Nit: some exceptions ends with ".", some with "!", some with no punctuation
at all. Let's have some consistency in this area.
5. Nit: I'd replace "Unknown dynamic path lookup error" to sth like "Unknown
queue path validation result"
6. {{MappingRuleValidationContextImpl}}: I'd definitely call {{hasNext()}}
before calling {{String staticPart = pointer.next();}} just to be 100% safe
7. {{MappingRuleValidationContextImpl}}: {{staticPartParent}} is initially
{{null}}. Any change of it remaining {{null}} after the {{while}} loop? If so,
a null check is warranted.
8. Nit: I'd rename "QueueCreationValidation" to
"QueueCreationValidationResult" or "AutoQueueCreateValidationResult" to reflect
that it is a result of something. Or simply "ValidationResult", because it's a
nested enum.
I can't really comment on the validation logic because there a lot of string
operations and hopefully the unit tests will do their job.
Talking about unit tests: {{TestMappingRuleValidationContextImpl}} was extended
with some new cases, does that provide enough coverage? Have you run it through
a coverage tool? EDIT: ok, the patch does not compile yet, so probably the
answer is "no".
Also, many checkstyle issue, pls fix them if time permits (mostly indentation).
was (Author: pbacsko):
Initial review:
1. Remove star imports: {{import java.util.*;}}
2. Remove star imports: {{import
org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.*;}}
3. Is this class used? {{import org.apache.hadoop.yarn.webapp.hamlet2.Hamlet;}}
4. Nit: some exceptions ends with ".", some with "!", some with no punctuation
at all. Let's have some consistency in this area.
5. Nit: I'd replace "Unknown dynamic path lookup error" to sth like "Unknown
queue path validation result"
6. {{MappingRuleValidationContextImpl}}: I'd definitely call {{hasNext()}}
before calling {{String staticPart = pointer.next();}} just to be 100% safe
7. {{MappingRuleValidationContextImpl}}: {{staticPartParent}} is initially
{{null}}. Any change of it remaining {{null}} after the {{while}} loop? If so,
a null check is warranted.
8. Nit: I'd rename "QueueCreationValidation" to
"QueueCreationValidationResult" or "AutoQueueCreateValidationResult" to reflect
that it is a result of something. Or simply "ValidationResult", because it's a
nested enum.
I can't really comment on the validation logic because there a lot of string
operations and hopefully the unit tests do their job.
Talking about unit tests: {{TestMappingRuleValidationContextImpl}} was extended
with some new cases, does that provide enough coverage? Have you run it through
a coverage tool?
Also, many checkstyle issue, pls fix them if time permits (mostly indentation).
> Make queue placement in CapacityScheduler compliant with auto-queue-placement
> -
>
> Key: YARN-10535
> URL: https://issues.apache.org/jira/browse/YARN-10535
> Project: Hadoop YARN
> Issue Type: Sub-task
> Components: capacity scheduler
>Reporter: Wangda Tan
>Assignee: Gergely Pollak
>Priority: Major
> Attachments: YARN-10535.001.patch
>
>
> Once YARN-10506 is done, we need to call the API from the queue placement
> policy to create queues.
--
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
