- 
                Notifications
    You must be signed in to change notification settings 
- Fork 3.4k
HBASE-25739 TableSkewCostFunction need to use aggregated deviation #3067
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| This is the the fix that only update table skew cost function | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| 💔 -1 overall 
 
 This message was automatically generated. | 
| 💔 -1 overall 
 
 This message was automatically generated. | 
9008479    to
    a456b01      
    Compare
  
    | 🎊 +1 overall 
 
 This message was automatically generated. | 
| 💔 -1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff to address else patch looks good.
        
          
                hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
          
            Show resolved
            Hide resolved
        
              
          
                hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
          
            Show resolved
            Hide resolved
        
              
          
                hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
              
          
                hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
              
                Outdated
          
            Show resolved
            Hide resolved
        
      | 🎊 +1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Has this been tried in production @clarax ?
Leaving open another day or so in case @d-c-manning or @sunhelly have some input... Thanks.
| And how does this relate to #3011 ? Thanks. | 
| 
 This was validated on a 12 node 110 region test cluster and a ~1000 node 400,000 region production cluster. | 
| 
 It is a sub task of HBASE-25625: https://issues.apache.org/jira/browse/HBASE-25739?filter=-2 | 
| Good. Thanks. | 
d7849df    to
    87e4588      
    Compare
  
    | updated the commit message to clarify this is for subtask https://issues.apache.org/jira/browse/HBASE-25739?filter=-2 | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| 🎊 +1 overall 
 
 This message was automatically generated. | 
| I will look today. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for the improvement!
| Hi there @d-c-manning , do you have a name an email address to which you'd like to receive attribution for your review? We try to honor the contributions made by reviewers through the use of  | 
| oh wow thanks @ndimiduk! - is there a way to make this available for you automatically?  | 
| This can be merged now? | 
| Merged to master. Thank you for the patch, @clarax , and for the careful review, @saintstack and @d-c-manning ! | 
| 
 I don't know of a means for automated attribution no. I look for this information in the Github user profile, and one around to ask if I don't find it. | 
| @apurtell are you comfortable with this as a backport to 2.4? I'm thinking it'd be good for 2.3... | 
…pache#3067) Signed-off-by: Michael Stack <[email protected]> Reviewed-by: David Manning <[email protected]>
| @infraio are you comfortable with this as a backport to 2.2? | 
…pache#3067) Signed-off-by: Michael Stack <[email protected]> Reviewed-by: David Manning <[email protected]>
…pache#3067) Signed-off-by: Michael Stack <[email protected]> Reviewed-by: David Manning <[email protected]>
…pache#3067) Signed-off-by: Michael Stack <[email protected]> Reviewed-by: David Manning <[email protected]>
…ation (apache#3067)" This reverts commit 533c84d.
…er way to evaluate resource distribution