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/08/14 07:02:07 UTC
[GitHub] [druid] ArvinZheng opened a new pull request #10280: recreate the balancer executor only when needed
ArvinZheng opened a new pull request #10280:
URL: https://github.com/apache/druid/pull/10280
Fixes #10273
----------------------------------------------------------------
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] ArvinZheng commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486761422
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
----------------------------------------------------------------
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] maytasm commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486676943
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
nit: same here as above
----------------------------------------------------------------
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] capistrant commented on pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on pull request #10280:
URL: https://github.com/apache/druid/pull/10280#issuecomment-693617905
Thanks for the contribution @ArvinZheng !
If you are interested in other PRs regarding coordinator resource usage/performance, I have a few open here that I would appreciate review/feedback on:
* https://github.com/apache/druid/pull/10287
* https://github.com/apache/druid/pull/10284
----------------------------------------------------------------
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] ArvinZheng commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486761307
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
----------------------------------------------------------------
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] capistrant commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r485205660
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -653,8 +683,8 @@ private void stopBeingLeader()
private final long startTimeNanos = System.nanoTime();
private final List<CoordinatorDuty> duties;
private final int startingLeaderCounter;
- private int cachedBalancerThreadNumber;
- private ListeningExecutorService balancerExec;
+ //private int cachedBalancerThreadNumber;
Review comment:
unused variables should be removed instead of commented
----------------------------------------------------------------
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] capistrant commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r471146224
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -733,11 +762,24 @@ public void run()
catch (Exception e) {
log.makeAlert(e, "Caught exception, ignoring so that schedule keeps going.").emit();
}
- finally {
- if (balancerExec != null) {
- balancerExec.shutdownNow();
Review comment:
do we need to worry about abandoning resources after removing this? Should the exec possibly be shutdown in stopBeingLeader() in your patch?
----------------------------------------------------------------
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] maytasm commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486677649
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
LGTM. Maybe add `@VisibleForTesting` too
----------------------------------------------------------------
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] ArvinZheng commented on pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on pull request #10280:
URL: https://github.com/apache/druid/pull/10280#issuecomment-695749701
> Thanks for the contribution @ArvinZheng !
>
> If you are interested in other PRs regarding coordinator resource usage/performance, I have a few open here that I would appreciate review/feedback on:
>
> * #10287
> * #10284
Thanks for the review, sure, I'll take a look when I get a chance.
----------------------------------------------------------------
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] ArvinZheng commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486761307
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
----------------------------------------------------------------
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] ArvinZheng commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486761307
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
----------------------------------------------------------------
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] ArvinZheng commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486761307
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
----------------------------------------------------------------
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] maytasm commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486676862
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
nit: comment is not needed since we already have @VisibleForTesting annotation
----------------------------------------------------------------
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] capistrant commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
capistrant commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r485145022
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
I see DutiesRunnable used to be public with a protected constructor but was flipped to private/private in [pull 9644](https://github.com/apache/druid/pull/9644) ... Now we are flipping the class and constructor to protected. I want to make sure @maytasm sees this PR and can comment on the idea of going back to protected from private.
----------------------------------------------------------------
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] maytasm commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
maytasm commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486677649
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
LGTM. Maybe add @VisibleForTesting too
----------------------------------------------------------------
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] ArvinZheng commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r485913685
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -653,8 +683,8 @@ private void stopBeingLeader()
private final long startTimeNanos = System.nanoTime();
private final List<CoordinatorDuty> duties;
private final int startingLeaderCounter;
- private int cachedBalancerThreadNumber;
- private ListeningExecutorService balancerExec;
+ //private int cachedBalancerThreadNumber;
Review comment:
oh, good catch, will remove them, thanks for reviewing
----------------------------------------------------------------
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] ArvinZheng commented on a change in pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
ArvinZheng commented on a change in pull request #10280:
URL: https://github.com/apache/druid/pull/10280#discussion_r486761307
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
updated
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -483,6 +487,24 @@ public void moveSegment(
}
}
+ /**
+ * This method should be used for only testing.
+ */
+ @VisibleForTesting
+ public int getCachedBalancerThreadNumber()
+ {
+ return cachedBalancerThreadNumber;
+ }
+
+ /**
+ * This method should be used for only testing.
Review comment:
addressed
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/DruidCoordinator.java
##########
@@ -647,22 +678,53 @@ private void stopBeingLeader()
return ImmutableList.of(compactSegments);
}
- private class DutiesRunnable implements Runnable
+ protected class DutiesRunnable implements Runnable
Review comment:
added, and thanks for your time.
----------------------------------------------------------------
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] capistrant merged pull request #10280: recreate the balancer executor only when needed
Posted by GitBox <gi...@apache.org>.
capistrant merged pull request #10280:
URL: https://github.com/apache/druid/pull/10280
----------------------------------------------------------------
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