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 2021/01/30 01:40:35 UTC
[GitHub] [druid] a2l007 opened a new pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
a2l007 opened a new pull request #10213:
URL: https://github.com/apache/druid/pull/10213
Fixes #10193.
`CuratorLoadQueuePeon` no longer deletes segment load/drop entries in case `druid.coordinator.load.timeout` expires. Deleting these entries after a timeout can cause the balancer to work incorrectly, as described in the linked issue.
With this fix, the segment entries will remain in the load/drop queue for a peon until the ZK entry is deleted by the historical, unless a non-timeout related exception occurs. This helps the balancer to account for the actual queue size for historicals and can lead to better balancing decisions.
<hr>
This PR has:
- [x] been self-reviewed.
- [x] using the [concurrency checklist](https://github.com/apache/druid/blob/master/dev/code-review/concurrency.md) (Remove this item if the PR doesn't have any relation to concurrency.)
- [x] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [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.
- [x] been tested in a test Druid cluster.
<!-- Check the items by putting "x" in the brackets for the done things. Not all of these items apply to every PR. Remove the items which are not done or not relevant to the PR. None of the items from the checklist above are strictly necessary, but it would be very helpful if you at least self-review the 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] stale[bot] closed pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
stale[bot] closed pull request #10213:
URL: https://github.com/apache/druid/pull/10213
----------------------------------------------------------------
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 a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r591845229
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
##########
@@ -59,7 +59,10 @@ public static SegmentReplicantLookup make(DruidCluster cluster)
if (numReplicants == null) {
numReplicants = 0;
}
- loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
+ // Timed out segments need to be replicated in another server for faster availability
+ if (!serverHolder.getPeon().getTimedOutSegments().contains(segment)) {
+ loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
Review comment:
Adding a config seems reasonable to me :+1:
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r594562633
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/CuratorLoadQueuePeon.java
##########
@@ -361,21 +386,33 @@ private void entryRemoved(SegmentHolder segmentHolder, String path)
);
}
- private void failAssign(SegmentHolder segmentHolder)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout)
{
- failAssign(segmentHolder, null);
+ failAssign(segmentHolder, handleTimeout, null);
}
- private void failAssign(SegmentHolder segmentHolder, Exception e)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout, Exception e)
{
if (e != null) {
log.error(e, "Server[%s], throwable caught when submitting [%s].", basePath, segmentHolder);
Review comment:
> Alerting sounds like a good idea, but my concern is that since the alert would happen per segment, a slowness on the historical side can generate a large number of alerts for a fairly large cluster. What do you think?
I think it's a valid concern. We may be able to emit those exceptions in bulk if they are thrown in a short time frame. I believe this should be done in a separate PR even if we want, and thus my comment is not a blocker for this PR.
> Also as a followup PR I was planning to add the timedOut segment list to the `/druid/coordinator/v1/loadqueue` along with some docs about its usage in understanding the cluster behavior.
Thanks. It sounds good to me.
----------------------------------------------------------------
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] stale[bot] commented on pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10213:
URL: https://github.com/apache/druid/pull/10213#issuecomment-703191260
This pull request has been marked as stale due to 60 days of inactivity. It will be closed in 4 weeks if no further activity occurs. If you think that's incorrect or this pull request should instead be reviewed, please simply write any comment. Even if closed, you can still revive the PR at any time or discuss it on the dev@druid.apache.org list. Thank you for your contributions.
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r596285796
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
##########
@@ -59,7 +59,10 @@ public static SegmentReplicantLookup make(DruidCluster cluster)
if (numReplicants == null) {
numReplicants = 0;
}
- loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
+ // Timed out segments need to be replicated in another server for faster availability
+ if (!serverHolder.getPeon().getTimedOutSegments().contains(segment)) {
+ loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
Review comment:
It sounds good to me to preserve the existing behavior by default.
----------------------------------------------------------------
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 a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r590528680
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/CuratorLoadQueuePeon.java
##########
@@ -361,21 +386,33 @@ private void entryRemoved(SegmentHolder segmentHolder, String path)
);
}
- private void failAssign(SegmentHolder segmentHolder)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout)
{
- failAssign(segmentHolder, null);
+ failAssign(segmentHolder, handleTimeout, null);
}
- private void failAssign(SegmentHolder segmentHolder, Exception e)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout, Exception e)
{
if (e != null) {
log.error(e, "Server[%s], throwable caught when submitting [%s].", basePath, segmentHolder);
Review comment:
Alerting sounds like a good idea, but my concern is that since the alert would happen per segment, a slowness on the historical side can generate a large number of alerts for a fairly large cluster. What do you think?
----------------------------------------------------------------
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] stale[bot] commented on pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10213:
URL: https://github.com/apache/druid/pull/10213#issuecomment-727174809
This pull request/issue has been closed due to lack of activity. If you think that is incorrect, or the pull request requires review, you can revive the PR at any 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] a2l007 commented on a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r590535621
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
##########
@@ -59,7 +59,10 @@ public static SegmentReplicantLookup make(DruidCluster cluster)
if (numReplicants == null) {
numReplicants = 0;
}
- loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
+ // Timed out segments need to be replicated in another server for faster availability
+ if (!serverHolder.getPeon().getTimedOutSegments().contains(segment)) {
+ loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
Review comment:
@jihoonson @himanshug Would it make sense to make the replication behavior user configurable? We could have a dynamic config like `replicateAfterLoadTimeout` which would control whether the segments would be attempted to be replicated to a different historical in case of a load timeout to the current historical. The default could be true but a cluster operator can set this to false if they wish to avoid the additional churn and know the historicals are OK and it would eventually load the segments.
----------------------------------------------------------------
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 a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
clintropolis commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r581022989
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/CuratorLoadQueuePeon.java
##########
@@ -282,14 +297,14 @@ public void run()
() -> {
try {
if (curator.checkExists().forPath(path) != null) {
- failAssign(segmentHolder, new ISE("%s was never removed! Failing this operation!", path));
+ failAssign(segmentHolder, true, new ISE("%s was never removed! Failing this operation!", path));
Review comment:
I think it would be worth clarifying this log message to indicate that for load operations, that while the coordinator has given up, the historical might still process and load the requested segments. Maybe something like "Load segments operation timed out, %s was never removed! Abandoning attempt, (but these segments might still be loaded)". I guess it would need to adjust message based on whether it was a load or drop.
----------------------------------------------------------------
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 a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r595471162
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
##########
@@ -59,7 +59,10 @@ public static SegmentReplicantLookup make(DruidCluster cluster)
if (numReplicants == null) {
numReplicants = 0;
}
- loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
+ // Timed out segments need to be replicated in another server for faster availability
+ if (!serverHolder.getPeon().getTimedOutSegments().contains(segment)) {
+ loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
Review comment:
Added a config. I've set `replicateAfterLoadTimeout` to false as the default I feel it might be better to preserve the existing behaviour and admins need to be aware of this property's behavior before setting it to true. Let me know what you think.
----------------------------------------------------------------
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 a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r590529034
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/CuratorLoadQueuePeon.java
##########
@@ -361,21 +386,33 @@ private void entryRemoved(SegmentHolder segmentHolder, String path)
);
}
- private void failAssign(SegmentHolder segmentHolder)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout)
{
- failAssign(segmentHolder, null);
+ failAssign(segmentHolder, handleTimeout, null);
}
- private void failAssign(SegmentHolder segmentHolder, Exception e)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout, Exception e)
{
if (e != null) {
log.error(e, "Server[%s], throwable caught when submitting [%s].", basePath, segmentHolder);
Review comment:
Also as a followup PR I was planning to add the timedOut segment list to the `/druid/coordinator/v1/loadqueue` along with some docs about its usage in understanding the cluster behavior.
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r589116331
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/CuratorLoadQueuePeon.java
##########
@@ -361,21 +386,33 @@ private void entryRemoved(SegmentHolder segmentHolder, String path)
);
}
- private void failAssign(SegmentHolder segmentHolder)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout)
{
- failAssign(segmentHolder, null);
+ failAssign(segmentHolder, handleTimeout, null);
}
- private void failAssign(SegmentHolder segmentHolder, Exception e)
+ private void failAssign(SegmentHolder segmentHolder, boolean handleTimeout, Exception e)
{
if (e != null) {
log.error(e, "Server[%s], throwable caught when submitting [%s].", basePath, segmentHolder);
Review comment:
I'm not sure why we don't emit exceptions currently (using `EmittingLogger.makeAlert()`), but should we? At least for the segment loading timeout error, it would be nice to emit those errors so that cluster operators can notice there is something going wrong with segment loading.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
##########
@@ -59,7 +59,10 @@ public static SegmentReplicantLookup make(DruidCluster cluster)
if (numReplicants == null) {
numReplicants = 0;
}
- loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
+ // Timed out segments need to be replicated in another server for faster availability
+ if (!serverHolder.getPeon().getTimedOutSegments().contains(segment)) {
+ loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
Review comment:
As @himanshug pointed out in https://github.com/apache/druid/issues/10193#issuecomment-790941778, there could be two types of slow segment loading.
- There are a few historicals being slow in segment loading in the cluster. This can be caused by unbalanced load queues or some intermittent failures.
- Historicals are OK, but ingestion might outpace the ability to load segments.
This particular change in `SegmentReplicantLookup` could help in the former case, but make things worse in the latter case. In an extreme case, all historicals could have the same set of timed-out segments in their load queue. This might be still OK though, because, if that's the case, Druid cannot get out of that state by itself anyway. The system administrator should add more historicals or use more threads for parallel segment loading. However, we should provide relevant data so that system administrators can tell what's happening. I left another comment about emitting exceptions to provide such data.
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
##########
@@ -59,7 +59,10 @@ public static SegmentReplicantLookup make(DruidCluster cluster)
if (numReplicants == null) {
numReplicants = 0;
}
- loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
+ // Timed out segments need to be replicated in another server for faster availability
+ if (!serverHolder.getPeon().getTimedOutSegments().contains(segment)) {
+ loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
Review comment:
`loadingSegments` is not just a set of segments loading anymore. Please add some javadoc in `SegmentReplicantLookup` about this.
----------------------------------------------------------------
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] jihoonson commented on a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
jihoonson commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r594562985
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/SegmentReplicantLookup.java
##########
@@ -59,7 +59,10 @@ public static SegmentReplicantLookup make(DruidCluster cluster)
if (numReplicants == null) {
numReplicants = 0;
}
- loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
+ // Timed out segments need to be replicated in another server for faster availability
+ if (!serverHolder.getPeon().getTimedOutSegments().contains(segment)) {
+ loadingSegments.put(segment.getId(), server.getTier(), numReplicants + 1);
Review comment:
It sounds good to me 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] jihoonson merged pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
jihoonson merged pull request #10213:
URL: https://github.com/apache/druid/pull/10213
----------------------------------------------------------------
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] stale[bot] commented on pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
stale[bot] commented on pull request #10213:
URL: https://github.com/apache/druid/pull/10213#issuecomment-770133373
----------------------------------------------------------------
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 a change in pull request #10213: Avoid deletion of load/drop entry from CuratorLoadQueuePeon in case of load timeout
Posted by GitBox <gi...@apache.org>.
a2l007 commented on a change in pull request #10213:
URL: https://github.com/apache/druid/pull/10213#discussion_r586770325
##########
File path: server/src/main/java/org/apache/druid/server/coordinator/CuratorLoadQueuePeon.java
##########
@@ -282,14 +297,14 @@ public void run()
() -> {
try {
if (curator.checkExists().forPath(path) != null) {
- failAssign(segmentHolder, new ISE("%s was never removed! Failing this operation!", path));
+ failAssign(segmentHolder, true, new ISE("%s was never removed! Failing this operation!", path));
Review comment:
I've modified the message here. Please let me know if this works.
----------------------------------------------------------------
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