[ 
https://issues.apache.org/jira/browse/YARN-10759?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17344573#comment-17344573
 ] 

Peter Bacsko edited comment on YARN-10759 at 5/14/21, 12:54 PM:
----------------------------------------------------------------

Thanks [~gandras] for the patch. I just have one thing to note.

I can see that {{allowZeroCapacitySum}} has been moved to {{AbstractCSQueue}}, 
although it's really something which is meant for {{ParentQueue}}. I assume 
this is because the new code is easier to read and no type checks and casts are 
necessary. Is that correct?

I'm wondering if this can cause problems. Because right now, this logic only 
runs inside {{ParentQueue}}:
{noformat}
                      // We also allow children's percent sum = 0 under the 
following           
                      // conditions             
                      // - Parent uses weight mode              
                      // - Parent uses percent mode, and parent has             
                      //   (capacity=0 OR allowZero)            
                      if (parentCapacityType == QueueCapacityType.PERCENT) {    
        
                        if ((Math.abs(queueCapacities.getCapacity(nodeLabel))   
        
                            > PRECISION) && (!allowZeroCapacitySum)) {          
                          throw new IOException(                
                              "Illegal" + " capacity sum of " + childrenPctSum  
        
                                  + " for children of queue " + queueName       
        
                                  + " for label=" + nodeLabel           
                                  + ". It is set to 0, but parent percent != 0, 
and "           
                                  + "doesn't allow children capacity to set to 
0");             
                        }               
                      }         
                    }
{noformat}
But after this refactor, leaf queues will have this property too with it being 
set to "false". Although there are no unit test failures, we need to double 
check if this extra boolean flag on leafs can have any impact on the existing 
code.


was (Author: pbacsko):
Thanks [~gandras] for the patch. I just have one to note.

I can see that {{allowZeroCapacitySum}} has been moved to {{AbstractCSQueue}}, 
although it's really something which is meant for {{ParentQueue}}. I assume 
this is because the new code is easier to read and no type checks and casts are 
necessary. Is that correct?

I'm wondering if this can cause problems. Because right now, this logic only 
runs inside {{ParentQueue}}:
{noformat}
                      // We also allow children's percent sum = 0 under the 
following           
                      // conditions             
                      // - Parent uses weight mode              
                      // - Parent uses percent mode, and parent has             
                      //   (capacity=0 OR allowZero)            
                      if (parentCapacityType == QueueCapacityType.PERCENT) {    
        
                        if ((Math.abs(queueCapacities.getCapacity(nodeLabel))   
        
                            > PRECISION) && (!allowZeroCapacitySum)) {          
                          throw new IOException(                
                              "Illegal" + " capacity sum of " + childrenPctSum  
        
                                  + " for children of queue " + queueName       
        
                                  + " for label=" + nodeLabel           
                                  + ". It is set to 0, but parent percent != 0, 
and "           
                                  + "doesn't allow children capacity to set to 
0");             
                        }               
                      }         
                    }
{noformat}
But after this refactor, leaf queues will have this property too with it being 
set to "false". Although there are no unit test failures, we need to double 
check if this extra boolean flag on leafs can have any impact on the existing 
code.

> Encapsulate queue config modes
> ------------------------------
>
>                 Key: YARN-10759
>                 URL: https://issues.apache.org/jira/browse/YARN-10759
>             Project: Hadoop YARN
>          Issue Type: Improvement
>          Components: capacity scheduler
>            Reporter: Andras Gyori
>            Assignee: Andras Gyori
>            Priority: Major
>         Attachments: YARN-10759.001.patch, YARN-10759.002.patch, 
> YARN-10759.003.patch, YARN-10759.004.patch
>
>
> Capacity Scheduler queues have three modes:
>  * relative/percentage
>  * weight
>  * absolute
> Most of them have their own:
>  * validation logic
>  * config setting logic
>  * effective capacity calculation logic
> These logics can be easily extracted and encapsulated in separate config mode 
> classes. 



--
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

Reply via email to