[jira] [Commented] (YARN-10428) Zombie applications in the YARN queue using FAIR + sizebasedweight
[ https://issues.apache.org/jira/browse/YARN-10428?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17267048#comment-17267048 ] Andras Gyori commented on YARN-10428: - Hey [~yguang11], thanks for the patch. We have encountered the same issue. Do you know any easy way to reproduce in a deterministic way? I would like to explore the possibilities and also understand the root cause. > Zombie applications in the YARN queue using FAIR + sizebasedweight > -- > > Key: YARN-10428 > URL: https://issues.apache.org/jira/browse/YARN-10428 > Project: Hadoop YARN > Issue Type: Bug > Components: capacityscheduler >Affects Versions: 2.8.5 >Reporter: Guang Yang >Priority: Major > Attachments: YARN-10428.001.patch, YARN-10428.002.patch > > > Seeing zombie jobs in the YARN queue that uses FAIR and size based weight > ordering policy . > *Detection:* > The YARN UI shows incorrect number of "Num Schedulable Applications". > *Impact:* > The queue has an upper limit of number of running applications, with zombie > job, it hits the limit even though the number of running applications is far > less than the limit. > *Workaround:* > **Fail-over and restart Resource Manager process. > *Analysis:* > **In the heap dump, we can find the zombie jobs in the `FairOderingPolicy# > schedulableEntities` (see attachment). Take application > "application_1599157165858_29429" for example, it is still in the > `FairOderingPolicy#schedulableEntities` set, however, if we check the log of > resource manager, we can see RM already tried to remove the application: > > ./yarn-yarn-resourcemanager-ip-172-21-153-252.log.2020-09-04-04:2020-09-04 > 04:32:19,730 INFO > org.apache.hadoop.yarn.server.resourcemanager.scheduler.capacity.LeafQueue > (ResourceManager Event Processor): Application removed - appId: > application_1599157165858_29429 user: svc_di_data_eng queue: core-data > #user-pending-applications: -3 #user-active-applications: 7 > #queue-pending-applications: 0 #queue-active-applications: 21 > > So it appears RM failed to removed the application from the set. -- 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-10572) Merge YARN-8557 and YARN-10352, and rebase based YARN-10380.
[ https://issues.apache.org/jira/browse/YARN-10572?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17264155#comment-17264155 ] zhuqi edited comment on YARN-10572 at 1/18/21, 6:30 AM: [~wangda] [~bibinchundatt] [~prabhujoseph] [~tangzhankun] I have updated a patch to rebase YARN-10352 and merge the difference in YARN-8557. Also refactor some method. If you could review and merge it? was (Author: zhuqi): [~wangda] [~bibinchundatt] [~prabhujoseph] I have updated a patch to rebase YARN-10352 and merge the difference in YARN-8557. Also refactor some method. If you could review and merge it? > Merge YARN-8557 and YARN-10352, and rebase based YARN-10380. > > > Key: YARN-10572 > URL: https://issues.apache.org/jira/browse/YARN-10572 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: zhuqi >Assignee: zhuqi >Priority: Major > Attachments: YARN-10572.001.patch > > > The work is : > 1. Because of YARN-10380, We should rebase YARN-10352 > 2. Also merge YARN-8557 for not running case skip. > 3. Refactor some method in YARN-10380 -- 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-10532) Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is not being used
[ https://issues.apache.org/jira/browse/YARN-10532?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17266529#comment-17266529 ] zhuqi edited comment on YARN-10532 at 1/18/21, 5:47 AM: [~wangda] [~gandras] I add a draft poc patch for 1, 2, 3 above. The expired time is just used for old auto created leaf queue, i reuse the expired logic to trigger old auto created expired for deletion. And the new auto created leaf queue , i have used the submit time. I will fix and deep into some other cases for more details. Thanks. was (Author: zhuqi): [~wangda] [~gandras] I add a draft poc patch for 1, 2, 3 above. I will fix and deep into some other cases for more details. Thanks. > Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is > not being used > > > Key: YARN-10532 > URL: https://issues.apache.org/jira/browse/YARN-10532 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Wangda Tan >Assignee: zhuqi >Priority: Major > Attachments: YARN-10532.001.patch, YARN-10532.002.patch, > YARN-10532.003.patch > > > It's better if we can delete auto-created queues when they are not in use for > a period of time (like 5 mins). It will be helpful when we have a large > number of auto-created queues (e.g. from 500 users), but only a small subset > of queues are actively used. -- 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-10532) Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is not being used
[
https://issues.apache.org/jira/browse/YARN-10532?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17267022#comment-17267022
]
zhuqi edited comment on YARN-10532 at 1/18/21, 5:42 AM:
The latest patch support:
For old auto created leaf queue deletion:
1. Support policy based auto deletion for expired queue:
For old auto created leaf queue:
1.1 Support GuaranteedOrZeroCapacityOverTimePolicy based deletion :
{code:java}
long lastActive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentActivationTime();
long lastDeactive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentDeactivationTime();
// Check if need delete when expired.
if (lastActive >= lastDeactive
|| (lastDeactive - lastActive)/1000 <=
scheduler.getConfiguration().
getAutoExpiredDeletionTime(managedParentQueue.getQueuePath())
|| leafQueue.getAllApplications().size() > 0) {
isExpired = false;
}
{code}
For new auto created leaf queue:
{code:java}
private synchronized void computeDynamicLeafQueueChanges(LeafQueue leafQueue)
throws SchedulerDynamicEditException {
// Expired queue, when there are no running in leafQueue
// and the last submit time has been expired
// Delete queue when expired deletion enabled.
ParentQueue parentQueue = (ParentQueue) leafQueue.getParent();
if (parentQueue == null) {
throw new SchedulerDynamicEditException("Parent " +
"queue should not be null for auto deletion!");
}
long idleDuration =
(System.currentTimeMillis()
- leafQueue.getLastSubmittedTimestamp())/1000;
if (leafQueue.getAllApplications().size() ==0
&& idleDuration > this.getConfiguration()
.getAutoExpiredDeletionTime(leafQueue.getParent().getQueuePath())
&& this.getConfiguration().
isAutoExpiredDeletionEnabled(leafQueue.getParent().getQueuePath())){
LeafQueue removed = parentQueue.
removeDynamicLeafQueue(leafQueue.getQueuePath());
if (removed != null) {
this.getCapacitySchedulerQueueManager().
removeQueue(leafQueue.getQueuePath());
}
}
}
{code}
2. Support policy not enabled with Reinitialize update deletion:
{code:java}
private void updateQueues(CSQueueStore existingQueues,
CSQueueStore newQueues) {
CapacitySchedulerConfiguration conf = csContext.getConfiguration();
for (CSQueue queue : newQueues.getQueues()) {
if (existingQueues.get(queue.getQueuePath()) == null) {
existingQueues.add(queue);
}
}
for (CSQueue queue : existingQueues.getQueues()) {
// should also support for auto created for expired deletion
// 1. handle old auto created deletion for reinitializeQueues
// 2. handle new auto created deletion for reinitializeQueues
if ((queue.getParent() != null && queue instanceof AutoCreatedLeafQueue &&
conf.isAutoExpiredDeletionEnabled(queue.getParent().getQueuePath())
&& (newQueues.get(queue.getQueuePath())) == null &&
((AutoCreatedLeafQueue) queue).isExpiredQueue())
||
(queue.getParent() != null && queue instanceof LeafQueue &&
((LeafQueue) queue).isDynamicQueue() &&
conf.isAutoExpiredDeletionEnabled(queue.getParent().getQueuePath())
&& (newQueues.get(queue.getQueuePath())) == null &&
((System.currentTimeMillis() -
((LeafQueue)queue).getLastSubmittedTimestamp())
>
conf.getAutoExpiredDeletionTime(queue.getParent().getQueuePath())) &&
((LeafQueue)queue).getAllApplications().size() == 0)
||
!((AbstractCSQueue) queue).isDynamicQueue() && newQueues.get(
queue.getQueuePath()) == null && !(
queue instanceof AutoCreatedLeafQueue && conf
.isAutoCreateChildQueueEnabled(
queue.getParent().getQueuePath( {
existingQueues.remove(queue);
}
}
{code}
Other remaining to do:
# If we need to support auto deletion also for parent queues.
# I will deep into more details about all the corner cases.
# The queue name / queue path make confused when deletion and some related
case.
was (Author: zhuqi):
The latest patch support:
For old auto created leaf queue deletion:
1. Support policy based auto deletion for expired queue:
For old auto created leaf queue:
1.1 Support GuaranteedOrZeroCapacityOverTimePolicy based deletion :
{code:java}
long lastActive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentActivationTime();
long lastDeactive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentDeactivationTime();
// Check if need delete when expired.
if (lastActive >= lastDeactive
|| (lastDeactive - lastActive)/1000 <=
scheduler.getConfiguration().
getAutoExpiredDeletionTime(managedParentQueue.getQueuePath())
|| leafQueue.getAllApplications().size() > 0) {
isExpired = false;
}
{code}
For new auto created leaf queue:
{code:java}
pri
[jira] [Comment Edited] (YARN-10532) Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is not being used
[
https://issues.apache.org/jira/browse/YARN-10532?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17267022#comment-17267022
]
zhuqi edited comment on YARN-10532 at 1/18/21, 5:42 AM:
[~wangda] [~gandras]
The latest patch support:
For old auto created leaf queue deletion:
1. Support policy based auto deletion for expired queue:
For old auto created leaf queue:
1.1 Support GuaranteedOrZeroCapacityOverTimePolicy based deletion :
{code:java}
long lastActive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentActivationTime();
long lastDeactive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentDeactivationTime();
// Check if need delete when expired.
if (lastActive >= lastDeactive
|| (lastDeactive - lastActive)/1000 <=
scheduler.getConfiguration().
getAutoExpiredDeletionTime(managedParentQueue.getQueuePath())
|| leafQueue.getAllApplications().size() > 0) {
isExpired = false;
}
{code}
For new auto created leaf queue:
{code:java}
private synchronized void computeDynamicLeafQueueChanges(LeafQueue leafQueue)
throws SchedulerDynamicEditException {
// Expired queue, when there are no running in leafQueue
// and the last submit time has been expired
// Delete queue when expired deletion enabled.
ParentQueue parentQueue = (ParentQueue) leafQueue.getParent();
if (parentQueue == null) {
throw new SchedulerDynamicEditException("Parent " +
"queue should not be null for auto deletion!");
}
long idleDuration =
(System.currentTimeMillis()
- leafQueue.getLastSubmittedTimestamp())/1000;
if (leafQueue.getAllApplications().size() ==0
&& idleDuration > this.getConfiguration()
.getAutoExpiredDeletionTime(leafQueue.getParent().getQueuePath())
&& this.getConfiguration().
isAutoExpiredDeletionEnabled(leafQueue.getParent().getQueuePath())){
LeafQueue removed = parentQueue.
removeDynamicLeafQueue(leafQueue.getQueuePath());
if (removed != null) {
this.getCapacitySchedulerQueueManager().
removeQueue(leafQueue.getQueuePath());
}
}
}
{code}
2. Support policy not enabled with Reinitialize update deletion:
{code:java}
private void updateQueues(CSQueueStore existingQueues,
CSQueueStore newQueues) {
CapacitySchedulerConfiguration conf = csContext.getConfiguration();
for (CSQueue queue : newQueues.getQueues()) {
if (existingQueues.get(queue.getQueuePath()) == null) {
existingQueues.add(queue);
}
}
for (CSQueue queue : existingQueues.getQueues()) {
// should also support for auto created for expired deletion
// 1. handle old auto created deletion for reinitializeQueues
// 2. handle new auto created deletion for reinitializeQueues
if ((queue.getParent() != null && queue instanceof AutoCreatedLeafQueue &&
conf.isAutoExpiredDeletionEnabled(queue.getParent().getQueuePath())
&& (newQueues.get(queue.getQueuePath())) == null &&
((AutoCreatedLeafQueue) queue).isExpiredQueue())
||
(queue.getParent() != null && queue instanceof LeafQueue &&
((LeafQueue) queue).isDynamicQueue() &&
conf.isAutoExpiredDeletionEnabled(queue.getParent().getQueuePath())
&& (newQueues.get(queue.getQueuePath())) == null &&
((System.currentTimeMillis() -
((LeafQueue)queue).getLastSubmittedTimestamp())
>
conf.getAutoExpiredDeletionTime(queue.getParent().getQueuePath())) &&
((LeafQueue)queue).getAllApplications().size() == 0)
||
!((AbstractCSQueue) queue).isDynamicQueue() && newQueues.get(
queue.getQueuePath()) == null && !(
queue instanceof AutoCreatedLeafQueue && conf
.isAutoCreateChildQueueEnabled(
queue.getParent().getQueuePath( {
existingQueues.remove(queue);
}
}
{code}
Other remaining to do:
# If we need to support auto deletion also for parent queues.
# I will deep into more details about all the corner cases.
# The queue name / queue path make confused when deletion and some related
case.
was (Author: zhuqi):
The latest patch support:
For old auto created leaf queue deletion:
1. Support policy based auto deletion for expired queue:
For old auto created leaf queue:
1.1 Support GuaranteedOrZeroCapacityOverTimePolicy based deletion :
{code:java}
long lastActive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentActivationTime();
long lastDeactive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentDeactivationTime();
// Check if need delete when expired.
if (lastActive >= lastDeactive
|| (lastDeactive - lastActive)/1000 <=
scheduler.getConfiguration().
getAutoExpiredDeletionTime(managedParentQueue.getQueuePath())
|| leafQueue.getAllApplications().size() > 0) {
isExpired = false;
}
{code}
For new auto created lea
[jira] [Commented] (YARN-10532) Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is not being used
[
https://issues.apache.org/jira/browse/YARN-10532?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17267022#comment-17267022
]
zhuqi commented on YARN-10532:
--
The latest patch support:
For old auto created leaf queue deletion:
1. Support policy based auto deletion for expired queue:
For old auto created leaf queue:
1.1 Support GuaranteedOrZeroCapacityOverTimePolicy based deletion :
{code:java}
long lastActive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentActivationTime();
long lastDeactive =
getLeafQueueState(leafQueue, nodeLabel).getMostRecentDeactivationTime();
// Check if need delete when expired.
if (lastActive >= lastDeactive
|| (lastDeactive - lastActive)/1000 <=
scheduler.getConfiguration().
getAutoExpiredDeletionTime(managedParentQueue.getQueuePath())
|| leafQueue.getAllApplications().size() > 0) {
isExpired = false;
}
{code}
For new auto created leaf queue:
{code:java}
private synchronized void computeDynamicLeafQueueChanges(LeafQueue leafQueue)
throws SchedulerDynamicEditException {
// Expired queue, when there are no running in leafQueue
// and the last submit time has been expired
// Delete queue when expired deletion enabled.
ParentQueue parentQueue = (ParentQueue) leafQueue.getParent();
if (parentQueue == null) {
throw new SchedulerDynamicEditException("Parent " +
"queue should not be null for auto deletion!");
}
long idleDuration =
(System.currentTimeMillis()
- leafQueue.getLastSubmittedTimestamp())/1000;
if (leafQueue.getAllApplications().size() ==0
&& idleDuration > this.getConfiguration()
.getAutoExpiredDeletionTime(leafQueue.getParent().getQueuePath())
&& this.getConfiguration().
isAutoExpiredDeletionEnabled(leafQueue.getParent().getQueuePath())){
LeafQueue removed = parentQueue.
removeDynamicLeafQueue(leafQueue.getQueuePath());
if (removed != null) {
this.getCapacitySchedulerQueueManager().
removeQueue(leafQueue.getQueuePath());
}
}
}
{code}
2. Support policy not enabled with Reinitialize update deletion:
{code:java}
private void updateQueues(CSQueueStore existingQueues,
CSQueueStore newQueues) {
CapacitySchedulerConfiguration conf = csContext.getConfiguration();
for (CSQueue queue : newQueues.getQueues()) {
if (existingQueues.get(queue.getQueuePath()) == null) {
existingQueues.add(queue);
}
}
for (CSQueue queue : existingQueues.getQueues()) {
// should also support for auto created for expired deletion
// 1. handle old auto created deletion for reinitializeQueues
// 2. handle new auto created deletion for reinitializeQueues
if ((queue.getParent() != null && queue instanceof AutoCreatedLeafQueue &&
conf.isAutoExpiredDeletionEnabled(queue.getParent().getQueuePath())
&& (newQueues.get(queue.getQueuePath())) == null &&
((AutoCreatedLeafQueue) queue).isExpiredQueue())
||
(queue.getParent() != null && queue instanceof LeafQueue &&
((LeafQueue) queue).isDynamicQueue() &&
conf.isAutoExpiredDeletionEnabled(queue.getParent().getQueuePath())
&& (newQueues.get(queue.getQueuePath())) == null &&
((System.currentTimeMillis() -
((LeafQueue)queue).getLastSubmittedTimestamp())
>
conf.getAutoExpiredDeletionTime(queue.getParent().getQueuePath())) &&
((LeafQueue)queue).getAllApplications().size() == 0)
||
!((AbstractCSQueue) queue).isDynamicQueue() && newQueues.get(
queue.getQueuePath()) == null && !(
queue instanceof AutoCreatedLeafQueue && conf
.isAutoCreateChildQueueEnabled(
queue.getParent().getQueuePath( {
existingQueues.remove(queue);
}
}
{code}
Other remaining to do:
# If we need to support auto deletion also for parent queues.
# I will deep into more details about all the corner cases.
> Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is
> not being used
>
>
> Key: YARN-10532
> URL: https://issues.apache.org/jira/browse/YARN-10532
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Wangda Tan
>Assignee: zhuqi
>Priority: Major
> Attachments: YARN-10532.001.patch, YARN-10532.002.patch,
> YARN-10532.003.patch
>
>
> It's better if we can delete auto-created queues when they are not in use for
> a period of time (like 5 mins). It will be helpful when we have a large
> number of auto-created queues (e.g. from 500 users), but only a small subset
> of queues are actively used.
--
This message was sent by Atlassian Jira
(v8.3.4#803005)
-
[jira] [Updated] (YARN-10532) Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is not being used
[ https://issues.apache.org/jira/browse/YARN-10532?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] zhuqi updated YARN-10532: - Attachment: YARN-10532.003.patch > Capacity Scheduler Auto Queue Creation: Allow auto delete queue when queue is > not being used > > > Key: YARN-10532 > URL: https://issues.apache.org/jira/browse/YARN-10532 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Wangda Tan >Assignee: zhuqi >Priority: Major > Attachments: YARN-10532.001.patch, YARN-10532.002.patch, > YARN-10532.003.patch > > > It's better if we can delete auto-created queues when they are not in use for > a period of time (like 5 mins). It will be helpful when we have a large > number of auto-created queues (e.g. from 500 users), but only a small subset > of queues are actively used. -- 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] [Commented] (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&focusedCommentId=17266941#comment-17266941
]
Gergely Pollak commented on YARN-10535:
---
[~snemeth] thank you for the review, a good portion of this work needs to be
merged into CapacityScheduler or CapacitySchedulerQueueManager, this will be
done as a separate effort, since it requires a bit more refactoring, and I
wanted this patch out soon, because without this YARN-10506 won't be able to
utilize it's new feature set. So for most of your point my answer will be "will
be fixed in a followup jira". :)
1) I find it overkill to create a separate method for a simple split. It is
technically a split which puts the result into an ArrayList, but later I'll try
to find a more centralized place for parsing the queuePath this way.
1.1-1.5) We already have a lot of queue path storing classes (at least 2, but
there might be 3), so we should create only one if possible, this might be the
good reason to merge whatever possible. I simply didn't want to introduce a new
one, but I agree we need to centralize the QueuePath operations in a dedicated
class.
1.6) I don't want to add a getGrandparent method, because YARN-10506 is written
in a way, it can support more depth than 2, so I would like to make this code
flexible during the next iteration. Currently we limit the max depth to 2, but
the underling code supports more depth, so should we. But we'll need some
getNthParent method, where n=1 is the parent, n=2 grandparent ... etc, so
generally I agree with the concept, but we need to increase flexibility, to
make the code future proof.
2) The confusing part here is that "dynamic" is overused. Originally I used
called static path when a TARGET path was full static eg. "root.someting.leaf"
and dynamic when it had variables which are to be substituted, eg.
"root.group.%primary_group". But YARN-10506 introduced the dynamic parent
concept, so dynamic now can mean 2 different thing in this context. The
validateXXXQueuePath (where XXX STRICTLY for dynamic/static) methods are only
used for TARGET path validation. And not for validating paths with dynamic
parents. I hope it makes sense.
3.1) Yes it could be, but still I prefer it dynamic, since it is used within
the context, so there is not much point in calling one VALIDATION helper method
without a context. Technically it could be, semantically I don't think it
should. Also this is something which should be in the QueueManager in the first
place, or something we should query about the queue, so the best would be this
to be in the CSQueue or in QueueManager. During refactor I'll remove it from
here for sure.
3.2) I had only 1 typo? Cool :) Thx for finding it
3.3) The WHOLE CLASS should go. And it will, as soon I can integrate it to QM,
however that requires some test helper refactors as well, since currently QM is
mocked in a good portion of the tests, and we need this functionality during
the tests, so this is the only reason it is in a separate class for now.
3.3.1) Please see my earlier repsonse regarding queue parsing classes.
3.3.2) I don't think an empty check should be a separate method, This is
literally a hasNext / throw check, I don't want to create a method with the
only purpose of throwing an exception.
3.3.3) The isPathStatic IS the dedicated method, the
staticPartBuffer.toString() is the argument. Again I think it is overkill to
move it to an other method.
{code:java}
THIS:
if (!isPathStatic(staticPartBuffer.toString())) {
return true;
}
Would become THIS:
if (!checkRoot(staticPartBuffer)) {
return true;
}
{code}
Also the stringBuffer is kind of a special representation here, so probably we
would still keep the staticPartBuffer.toString() call, so all in all we would
have an extra call on our stack
3.3.4) This is a special validation method which we need only once, we don't
need to store the static part of the queue, we do some operation on it, the we
won't touch it again, I see no reason to extract this, since it's mostly part
of the validation, and not really reusable.
3.3.5-3.3.6) The whole static part of a queue is a mapping rule specific
validation thing, we could make methods for all of it, but we would only use
those methods once, and we'd need a few extra fields. Generally I agree we
should keep the size of the functions minimal, but these are all steps in the
very same and quite unique validation process, with almost 0 reusability, hence
I kept all in one method.
4) Will take a look at it, actually it might result in false positive result,
thx!
Thank you again for the detailed feedbacks, I'll fix the most urgent issues
here, and then keep the rest in mind for the followup tasks.
> Make queue placement in CapacityScheduler compliant with auto-queue-placement
> --
[jira] [Resolved] (YARN-10575) Hadoop
[ https://issues.apache.org/jira/browse/YARN-10575?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ] Wei-Chiu Chuang resolved YARN-10575. Resolution: Invalid > Hadoop > -- > > Key: YARN-10575 > URL: https://issues.apache.org/jira/browse/YARN-10575 > Project: Hadoop YARN > Issue Type: Bug >Reporter: Pushpalatha S K >Priority: Major > -- 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] [Commented] (YARN-10548) Decouple AM runner logic from SLSRunner
[
https://issues.apache.org/jira/browse/YARN-10548?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17266924#comment-17266924
]
Hadoop QA commented on YARN-10548:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
0s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
| {color:red}-1{color} | {color:red} patch {color} | {color:red} 0m 9s{color}
| {color:red}{color} | {color:red} YARN-10548 does not apply to trunk. Rebase
required? Wrong Branch? See https://wiki.apache.org/hadoop/HowToContribute for
help. {color} |
\\
\\
|| Subsystem || Report/Notes ||
| JIRA Issue | YARN-10548 |
| JIRA Patch URL |
https://issues.apache.org/jira/secure/attachment/13017678/YARN-10548.001.patch |
| Console output |
https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/498/console |
| versions | git=2.17.1 |
| Powered by | Apache Yetus 0.13.0-SNAPSHOT https://yetus.apache.org |
This message was automatically generated.
> Decouple AM runner logic from SLSRunner
> ---
>
> Key: YARN-10548
> URL: https://issues.apache.org/jira/browse/YARN-10548
> Project: Hadoop YARN
> Issue Type: Improvement
>Reporter: Szilard Nemeth
>Assignee: Szilard Nemeth
>Priority: Minor
> Attachments: YARN-10548.001.patch
>
>
> SLSRunner has too many responsibilities.
> One of them is to parse the job details from the SLS input formats and
> launch the AMs and task containers.
> The AM runner logic could be decoupled.
--
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&focusedCommentId=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 LeafQueu
[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&focusedCommentId=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 LeafQueu
[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&focusedCommentId=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 LeafQueu
[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&focusedCommentId=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 LeafQ
[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&focusedCommentId=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 Lea
[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&focusedCommentId=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 LeafQue
[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&focusedCommentId=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)
[jira] [Commented] (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&focusedCommentId=17266919#comment-17266919
]
Szilard Nemeth commented on YARN-10535:
---
*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 YarnException("Queue path '" + path +"' is invalid " +
"because '" + normalizedSta
[jira] [Commented] (YARN-10531) Be able to disable user limit factor for CapacityScheduler Leaf Queue
[
https://issues.apache.org/jira/browse/YARN-10531?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17266853#comment-17266853
]
Hadoop QA commented on YARN-10531:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 0m
46s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} || ||
| {color:green}+1{color} | {color:green} dupname {color} | {color:green} 0m
1s{color} | {color:green}{color} | {color:green} No case conflicting files
found. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch does not contain any
@author tags. {color} |
| {color:green}+1{color} | {color:green} {color} | {color:green} 0m 0s{color}
| {color:green}test4tests{color} | {color:green} The patch appears to include 2
new or modified test files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 20m
12s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 1m
1s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
53s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
51s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
57s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
16m 54s{color} | {color:green}{color} | {color:green} branch has no errors when
building and testing our client artifacts. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
42s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
40s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:blue}0{color} | {color:blue} spotbugs {color} | {color:blue} 1m
45s{color} | {color:blue}{color} | {color:blue} Used deprecated FindBugs
config; considering switching to SpotBugs. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 1m
43s{color} |
{color:red}https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/497/artifact/out/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-warnings.html{color}
| {color:red}
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
in trunk has 1 extant findbugs warnings. {color} |
|| || || || {color:brown} Patch Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
49s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
52s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
52s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
44s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
44s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:orange}-0{color} | {color:orange} checkstyle {color} | {color:orange}
0m 44s{color} |
{color:orange}https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/497/artifact/out/diff-checkstyle-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager.txt{color}
| {color:orange}
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:
The patch generated 2 new + 555 unchanged - 1 fixed = 557 total (was 556)
{color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
47s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{colo
[jira] [Commented] (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&focusedCommentId=17266809#comment-17266809
]
Hadoop QA commented on YARN-10535:
--
| (x) *{color:red}-1 overall{color}* |
\\
\\
|| Vote || Subsystem || Runtime || Logfile || Comment ||
| {color:blue}0{color} | {color:blue} reexec {color} | {color:blue} 1m
12s{color} | {color:blue}{color} | {color:blue} Docker mode activated. {color} |
|| || || || {color:brown} Prechecks {color} || ||
| {color:green}+1{color} | {color:green} dupname {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} No case conflicting files
found. {color} |
| {color:green}+1{color} | {color:green} @author {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch does not contain any
@author tags. {color} |
| {color:green}+1{color} | {color:green} {color} | {color:green} 0m 0s{color}
| {color:green}test4tests{color} | {color:green} The patch appears to include 3
new or modified test files. {color} |
|| || || || {color:brown} trunk Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 22m
33s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
59s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
49s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
36s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
53s{color} | {color:green}{color} | {color:green} trunk passed {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
18m 2s{color} | {color:green}{color} | {color:green} branch has no errors when
building and testing our client artifacts. {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
39s{color} | {color:green}{color} | {color:green} trunk passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javadoc {color} | {color:green} 0m
35s{color} | {color:green}{color} | {color:green} trunk passed with JDK Private
Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:blue}0{color} | {color:blue} spotbugs {color} | {color:blue} 1m
50s{color} | {color:blue}{color} | {color:blue} Used deprecated FindBugs
config; considering switching to SpotBugs. {color} |
| {color:red}-1{color} | {color:red} findbugs {color} | {color:red} 1m
47s{color} |
{color:red}https://ci-hadoop.apache.org/job/PreCommit-YARN-Build/496/artifact/out/branch-findbugs-hadoop-yarn-project_hadoop-yarn_hadoop-yarn-server_hadoop-yarn-server-resourcemanager-warnings.html{color}
| {color:red}
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager
in trunk has 1 extant findbugs warnings. {color} |
|| || || || {color:brown} Patch Compile Tests {color} || ||
| {color:green}+1{color} | {color:green} mvninstall {color} | {color:green} 0m
51s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
53s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Ubuntu-11.0.9.1+1-Ubuntu-0ubuntu1.18.04 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
53s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} compile {color} | {color:green} 0m
45s{color} | {color:green}{color} | {color:green} the patch passed with JDK
Private Build-1.8.0_275-8u275-b01-0ubuntu1~18.04-b01 {color} |
| {color:green}+1{color} | {color:green} javac {color} | {color:green} 0m
45s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} checkstyle {color} | {color:green} 0m
30s{color} | {color:green}{color} | {color:green}
hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-resourcemanager:
The patch generated 0 new + 24 unchanged - 1 fixed = 24 total (was 25) {color}
|
| {color:green}+1{color} | {color:green} mvnsite {color} | {color:green} 0m
47s{color} | {color:green}{color} | {color:green} the patch passed {color} |
| {color:green}+1{color} | {color:green} whitespace {color} | {color:green} 0m
0s{color} | {color:green}{color} | {color:green} The patch has no whitespace
issues. {color} |
| {color:green}+1{color} | {color:green} shadedclient {color} | {color:green}
16m 29s{color} | {
[jira] [Commented] (YARN-10531) Be able to disable user limit factor for CapacityScheduler Leaf Queue
[
https://issues.apache.org/jira/browse/YARN-10531?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17266799#comment-17266799
]
zhuqi commented on YARN-10531:
--
[~wangda]
I have rebased it, i have also added a new auto created based test in latest
patch.
If you any other advice?
Thanks.
> Be able to disable user limit factor for CapacityScheduler Leaf Queue
> -
>
> Key: YARN-10531
> URL: https://issues.apache.org/jira/browse/YARN-10531
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Wangda Tan
>Assignee: zhuqi
>Priority: Major
> Attachments: YARN-10531.001.patch, YARN-10531.002.patch,
> YARN-10531.003.patch
>
>
> User limit factor is used to define max cap of how much resource can be
> consumed by single user.
> Under Auto Queue Creation context, it doesn't make much sense to set user
> limit factor, because initially every queue will set weight to 1.0, we want
> user can consume more resource if possible. It is hard to pre-determine how
> to set up user limit factor. So it makes more sense to add a new value (like
> -1) to indicate we will disable user limit factor
> Logic need to be changed is below:
> (Inside LeafQueue.java)
> {code}
> Resource maxUserLimit = Resources.none();
> if (schedulingMode == SchedulingMode.RESPECT_PARTITION_EXCLUSIVITY) {
> maxUserLimit = Resources.multiplyAndRoundDown(queueCapacity,
> getUserLimitFactor());
> } else if (schedulingMode == SchedulingMode.IGNORE_PARTITION_EXCLUSIVITY)
> {
> maxUserLimit = partitionResource;
> }
> {code}
--
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] [Updated] (YARN-10531) Be able to disable user limit factor for CapacityScheduler Leaf Queue
[
https://issues.apache.org/jira/browse/YARN-10531?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
zhuqi updated YARN-10531:
-
Attachment: YARN-10531.003.patch
> Be able to disable user limit factor for CapacityScheduler Leaf Queue
> -
>
> Key: YARN-10531
> URL: https://issues.apache.org/jira/browse/YARN-10531
> Project: Hadoop YARN
> Issue Type: Sub-task
>Reporter: Wangda Tan
>Assignee: zhuqi
>Priority: Major
> Attachments: YARN-10531.001.patch, YARN-10531.002.patch,
> YARN-10531.003.patch
>
>
> User limit factor is used to define max cap of how much resource can be
> consumed by single user.
> Under Auto Queue Creation context, it doesn't make much sense to set user
> limit factor, because initially every queue will set weight to 1.0, we want
> user can consume more resource if possible. It is hard to pre-determine how
> to set up user limit factor. So it makes more sense to add a new value (like
> -1) to indicate we will disable user limit factor
> Logic need to be changed is below:
> (Inside LeafQueue.java)
> {code}
> Resource maxUserLimit = Resources.none();
> if (schedulingMode == SchedulingMode.RESPECT_PARTITION_EXCLUSIVITY) {
> maxUserLimit = Resources.multiplyAndRoundDown(queueCapacity,
> getUserLimitFactor());
> } else if (schedulingMode == SchedulingMode.IGNORE_PARTITION_EXCLUSIVITY)
> {
> maxUserLimit = partitionResource;
> }
> {code}
--
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] [Created] (YARN-10575) Hadoop
Pushpalatha S K created YARN-10575: -- Summary: Hadoop Key: YARN-10575 URL: https://issues.apache.org/jira/browse/YARN-10575 Project: Hadoop YARN Issue Type: Bug Reporter: Pushpalatha S K -- 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] [Commented] (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&focusedCommentId=17266771#comment-17266771 ] Gergely Pollak commented on YARN-10535: --- 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&focusedCommentId=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] [Updated] (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:all-tabpanel ] Gergely Pollak updated YARN-10535: -- Attachment: YARN-10535.005.patch > 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] [Commented] (YARN-10574) Fix the FindBugs warning introduced in YARN-10506
[ https://issues.apache.org/jira/browse/YARN-10574?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17266769#comment-17266769 ] Gergely Pollak commented on YARN-10574: --- Test failure is unrelated, test seems to be flaky. Test have not been added, because there is no functionality change, only fixing a FindBugs recommendation, everything is expected to work exactly the same way. > Fix the FindBugs warning introduced in YARN-10506 > - > > Key: YARN-10574 > URL: https://issues.apache.org/jira/browse/YARN-10574 > Project: Hadoop YARN > Issue Type: Sub-task >Reporter: Gergely Pollak >Assignee: Gergely Pollak >Priority: Major > Attachments: YARN-10574.001.patch > > > Findbugs started to give warnings about an unnecessary null check, it's > better to get rid of it. -- 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
