You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@druid.apache.org by GitBox <gi...@apache.org> on 2020/06/24 19:55:46 UTC

[GitHub] [druid] pjain1 opened a new pull request #10070: Fix balancer strategy

pjain1 opened a new pull request #10070:
URL: https://github.com/apache/druid/pull/10070


   Fixes https://github.com/apache/druid/issues/10067, https://github.com/apache/druid/issues/10068 and https://github.com/apache/druid/issues/10069
   
   ### Description
   
   Found multiple bugs in balancer strategy which are described in the issues above.
   
   <hr>
   
   This PR has:
   - [x] been self-reviewed.
   - [x] added unit tests or modified existing tests to cover new code paths, ensuring the threshold for [code coverage](https://github.com/apache/druid/blob/master/dev/code-review/code-coverage.md) is met.
   
   <hr>
   
   ##### Key changed/added classes in this PR
    * `CostBalancerStrategy`
    * `RandomBalancerStrategy`
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] a2l007 commented on pull request #10070: Fix balancer strategy

Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10070:
URL: https://github.com/apache/druid/pull/10070#issuecomment-652063436


   I feel we should mention this in the release notes since it modifies the default behavior for `druid.server.maxSize`. For clusters having historicals with this property set to zero, the coordinator will longer assign segments to them. I've added clarification to the docs via #10105 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] pjain1 commented on a change in pull request #10070: Fix balancer strategy

Posted by GitBox <gi...@apache.org>.
pjain1 commented on a change in pull request #10070:
URL: https://github.com/apache/druid/pull/10070#discussion_r445302030



##########
File path: server/src/main/java/org/apache/druid/server/coordinator/RandomBalancerStrategy.java
##########
@@ -28,20 +28,22 @@
 import java.util.NavigableSet;
 import java.util.Set;
 import java.util.concurrent.ThreadLocalRandom;
+import java.util.stream.Collectors;
 
 public class RandomBalancerStrategy implements BalancerStrategy
 {
   @Override
   public ServerHolder findNewSegmentHomeReplicator(DataSegment proposalSegment, List<ServerHolder> serverHolders)

Review comment:
       It is marked as Nullable in super class `BalancerStrategy`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] pjain1 merged pull request #10070: Fix balancer strategy

Posted by GitBox <gi...@apache.org>.
pjain1 merged pull request #10070:
URL: https://github.com/apache/druid/pull/10070


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] a2l007 commented on pull request #10070: Fix balancer strategy

Posted by GitBox <gi...@apache.org>.
a2l007 commented on pull request #10070:
URL: https://github.com/apache/druid/pull/10070#issuecomment-652068807


   I agree. However, the docs give an impression that it maybe okay to set this to the default value, although it isn't. Have tried to fix this in the linked PR.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] nishantmonu51 commented on a change in pull request #10070: Fix balancer strategy

Posted by GitBox <gi...@apache.org>.
nishantmonu51 commented on a change in pull request #10070:
URL: https://github.com/apache/druid/pull/10070#discussion_r445155089



##########
File path: server/src/main/java/org/apache/druid/server/coordinator/RandomBalancerStrategy.java
##########
@@ -28,20 +28,22 @@
 import java.util.NavigableSet;
 import java.util.Set;
 import java.util.concurrent.ThreadLocalRandom;
+import java.util.stream.Collectors;
 
 public class RandomBalancerStrategy implements BalancerStrategy
 {
   @Override
   public ServerHolder findNewSegmentHomeReplicator(DataSegment proposalSegment, List<ServerHolder> serverHolders)

Review comment:
       minor nit:mark return type as nullable




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org

[GitHub] [druid] clintropolis commented on pull request #10070: Fix balancer strategy

Posted by GitBox <gi...@apache.org>.
clintropolis commented on pull request #10070:
URL: https://github.com/apache/druid/pull/10070#issuecomment-652065443


   > I feel we should mention this in the release notes since it modifies the default behavior for druid.server.maxSize. For clusters having historicals with this property set to zero, the coordinator will longer assign segments to them. I've added clarification to the docs via #10105
   
   Thanks for bringing this up, am working on draft of release notes right now and will be sure to add this. I don't think it is technically a change though, as far as I can tell it was always intended that the balancer uses the size historicals advertise in order to make segment placement decisions, just some flaws were causing the coordinator to assign them anyway I guess? I can't imagine the balancer was otherwise working if size was set to 0, since size is used pretty heavily to determine what can be moved and to where.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org