Blacklist workers if they fail for too many times by niketh · Pull Request #3643 · apache/druid

niketh · 2016-11-01T20:26:58Z

Blacklisting Workers if the tasks have failed continuously for the last 'n' times that can be specified by maxRetriesBeforeBlacklist
Added a cleanup period that will remove blacklisted nodes after a certain period taskBlackListCleanupPeriod
Once node is blacklisted it only remains blacklisted for a period taskBlackListBackoffTimeMillis

cheddar · 2016-11-02T17:35:45Z


+  public Collection<ImmutableWorkerInfo> getBlackListedWorkers()
+  {
+    return ImmutableList.copyOf(


Collections.unmodifiableSet() should be good enough.

cheddar · 2016-11-02T17:37:06Z

👍 on the code changes

I hit "approve", but then realized we should add documentation for the new parameters. So, that should be done.

cheddar · 2016-11-02T18:22:09Z

+
+  @JsonProperty
+  @Min(60000)
+  private long taskBlackListBackoffTimeMillis = 900000;


Can we switch this to a Period?

cheddar · 2016-11-02T18:23:10Z

 ```

+#### Blacklisted Workers
+If the workers fail tasks above a threshold, the overlord will blacklist these workers. No more than 20% of the nodes can be blacklisted.


Can we reference one of the config parameters here to make it a bit easier to track down the set of params on the config page (maybe even link to the config page).

nishantmonu51

left few comments,
In case user has configured autoscaling, It would also be nice to make autoscaling smarter to replace a blacklisted node, that can be done in a separate PR.

nishantmonu51 · 2016-11-03T19:36:49Z

 |`druid.indexer.runner.taskShutdownLinkTimeout`|How long to wait on a shutdown request to a middle manager before timing out|PT1M|
 |`druid.indexer.runner.pendingTasksRunnerNumThreads`|Number of threads to allocate pending-tasks to workers, must be at least 1.|1|
+|`druid.indexer.runner.maxRetriesBeforeBlacklist`|Number of times the middle manager can fail a task,  before it gets blacklisted, must be at least 1|5|
+|`druid.indexer.runner.taskBlackListBackoffTime`|How long to wait before a task is whitelisted again. This value should be greater that the value set for taskBlackListCleanupPeriod.|PT15M|


rename this to workerBlackListBackoffTime & workerBlackListCleanupPeriod and modify docs to make it clear that we are blacklisting workers instead of tasks.

nishantmonu51 · 2016-11-03T19:38:10Z

 ```

+#### Blacklisted Workers
+If the workers fail tasks above a threshold, the overlord will blacklist these workers. No more than 20% of the nodes can be blacklisted. Blacklisted nodes will be periodically whitelisted.


is max percentage of blacklist workers configurable ?
If not, can we make it configurable ?

nishantmonu51 · 2016-11-03T19:41:18Z

+    // Clean blacklisted workers if blacklisted time has elapsed
+    if(System.currentTimeMillis() - zkWorker.getLastCompletedTaskTime().getMillis() >
+            config.getTaskBlackListBackoffTime().toStandardDuration().getMillis()){
+      // White listing node


this should be an info level log

nishantmonu51 · 2016-11-03T19:42:19Z

 |`druid.indexer.runner.taskCleanupTimeout`|How long to wait before failing a task after a middle manager is disconnected from Zookeeper.|PT15M|
 |`druid.indexer.runner.taskShutdownLinkTimeout`|How long to wait on a shutdown request to a middle manager before timing out|PT1M|
 |`druid.indexer.runner.pendingTasksRunnerNumThreads`|Number of threads to allocate pending-tasks to workers, must be at least 1.|1|
+|`druid.indexer.runner.maxRetriesBeforeBlacklist`|Number of times the middle manager can fail a task,  before it gets blacklisted, must be at least 1|5|


mention that only consecutive failures will be considered.

nishantmonu51 · 2016-11-03T19:46:07Z

+
+    // After backOffTime the nodes are whitelisted
+    System.out.println("Sleeping for " + timeoutPeriod.toStandardDuration().getMillis());
+    Thread.sleep(2*timeoutPeriod.toStandardDuration().getMillis());


minor nit : would be nice if we can avoid sleep, helps in making test more deterministic on slow machines.

cheddar · 2016-11-04T17:57:29Z

Great comments @nishantmonu51

Niketh is on vacation, so probably won't get to these changes until he is back.

2. Rename variable

niketh · 2016-11-22T23:03:55Z

@nishantmonu51 @cheddar Thanks for your comments. Made the requested changes.

cheddar

I think these changes look good. It does look like you weren't able to remove the Thread.sleep from the test, it would be great if you could reply to Nishant's comment explaining why it's hard to remove that though.

cheddar · 2016-11-23T01:08:47Z

+  @JsonProperty
+  @Max(100)
+  @Min(0)
+  private int maxPercentageBlacklistWorkers = 20;


Might I suggest making this a double?

niketh · 2016-11-23T17:54:10Z

@cheddar @nishantmonu51 I wasn't able to remove the Thread.sleep, because i wanted to automatic removal of blacklisted workers once a stipulated time elapses. The thread.sleep here is very small

cheddar · 2016-11-23T23:14:57Z

👍

nishantmonu51

👍

* Blacklist workers if they fail for too many times * Adding documentation * Changing to timeout to period and updating docs * 1. Add configurable maxPercentageBlacklistWorkers 2. Rename variable * Change maxPercentageBlacklistWorkers to double * Remove thread.sleep

niketh force-pushed the blacklist-workers branch 2 times, most recently from 70d1639 to 4b744ef Compare November 1, 2016 20:33

niketh closed this Nov 1, 2016

Blacklist workers if they fail for too many times

97f0f72

niketh reopened this Nov 1, 2016

niketh force-pushed the blacklist-workers branch from 4b744ef to 97f0f72 Compare November 1, 2016 21:15

cheddar approved these changes Nov 2, 2016

View reviewed changes

Adding documentation

8fb1d18

cheddar reviewed Nov 2, 2016

View reviewed changes

niketh closed this Nov 2, 2016

niketh reopened this Nov 2, 2016

niketh force-pushed the blacklist-workers branch from c704494 to 3936f05 Compare November 2, 2016 21:11

Changing to timeout to period and updating docs

0d7ecd2

niketh force-pushed the blacklist-workers branch from 3936f05 to 0d7ecd2 Compare November 2, 2016 22:41

nishantmonu51 reviewed Nov 3, 2016

View reviewed changes

fjy added this to the 0.9.3 milestone Nov 4, 2016

fjy added the Improvement label Nov 4, 2016

1. Add configurable maxPercentageBlacklistWorkers

67c124e

2. Rename variable

cheddar approved these changes Nov 23, 2016

View reviewed changes

Change maxPercentageBlacklistWorkers to double

f90b3a9

Remove thread.sleep

6195513

cheddar assigned cheddar and nishantmonu51 Nov 23, 2016

nishantmonu51 approved these changes Nov 29, 2016

View reviewed changes

nishantmonu51 merged commit 2640d17 into apache:master Nov 29, 2016

gianm mentioned this pull request Dec 3, 2016

RemoteTaskRunnerConfig: Fix Guice error on startup. #3737

Merged

gianm mentioned this pull request Aug 1, 2017

Fix bugs in RTR related to blacklisting, change default worker strategy #4619

Merged

clambertus unassigned cheddar and nishantmonu51 Jul 6, 2018

himanshug mentioned this pull request Jun 16, 2019

[Proposal] : Better handling for *bad* worker nodes. #1615

Closed

doctording mentioned this pull request Dec 6, 2019

JavaScriptWorkerSelectStrategy is not equal sometimes #8975

Closed

Conversation

niketh commented Nov 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheddar commented Nov 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nishantmonu51 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cheddar commented Nov 4, 2016

Uh oh!

niketh commented Nov 22, 2016

Uh oh!

cheddar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

niketh commented Nov 23, 2016

Uh oh!

cheddar commented Nov 23, 2016

Uh oh!

nishantmonu51 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

niketh commented Nov 1, 2016 •

edited

Loading