HRTR: make pending task execution handling to go through all tasks on not finding worker slots by himanshug · Pull Request #8697 · apache/druid

himanshug · 2019-10-18T20:15:58Z

Description

HRTR currently hangs on to a task try finding worker for it, with #7066 as I segregate tasks based on their type (to workaround problem mentioned in #8622 ) .. it is possible to find no worker for one task while other tasks could still find a worker, so it needs to go through all tasks and try to find workers for them.

HRTR is an experimental task runner. RTR based on zookeeper doesn't have this problem.

Additionally added few more endpoints to HttpRemoteTaskRunnerResource to introspect state of HttpRemoteTaskRunner for debugging.

This PR has:

been self-reviewed.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths.
been tested in a test Druid cluster.

Key changed/added classes in this PR

HttpRemoteTaskRunner

lgtm-com · 2019-10-18T21:34:18Z

This pull request introduces 1 alert when merging 8cf89ca6b42fa72397ced3f6c25e8de89b086015 into 1ca8595 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

not finding worker slots

lgtm-com · 2019-11-06T19:40:29Z

This pull request introduces 1 alert when merging c8c1f40 into 7b77cf1 - view on LGTM.com

new alerts:

1 for Dereferenced variable may be null

himanshug · 2019-12-10T23:47:07Z

lgtm issue can be ignored in this case and I have been running this patch in prod for quite some time. in any case, we are probably only one using HRTR for now.

jihoonson · 2019-12-11T00:05:24Z

lgtm issue can be ignored in this case and I have been running this patch in prod for quite some time. in any case, we are probably only one using HRTR for now.

Hi @himanshug, I'm reviewing this PR. The LGTM warning is about the below code block.

        try {
          if (immutableWorker == null) {
            throw new ISE("NULL immutableWorker");
          }

          // this will send HTTP request to worker for assigning task
          if (!runTaskOnWorker(taskItem, immutableWorker.getWorker().getHost())) {
            if (taskItem.getState() == HttpRemoteTaskRunnerWorkItem.State.PENDING_WORKER_ASSIGN) {
              taskItem.revertStateFromPendingWorkerAssignToPending();
            }
          }
        }
        catch (InterruptedException ex) {
          log.info("Got InterruptedException while assigning task[%s].", taskId);
          throw ex;
        }
        catch (Throwable th) {
          log.makeAlert(th, "Exception while trying to assign task")
             .addData("taskId", taskId)
             .emit();

          taskComplete(taskItem, null, TaskStatus.failure(taskId));
        }
        finally {
          synchronized (statusLock) {
            workersWithUnacknowledgedTask.remove(immutableWorker.getWorker().getHost());
            statusLock.notifyAll();
          }
        }

It seems like immutableWorker can be null in the finally clause and so the LGTM warning looks legit to me. Would you explain more why this is ok and can be ignored?

jihoonson

@himanshug thanks for the fix. Left a couple of comments.

jihoonson · 2019-12-10T23:22:34Z

+   * Must not be used outside of this class and {@link HttpRemoteTaskRunnerResource}
+   */
+  @SuppressWarnings("GuardedBy") // Read on workersWithUnacknowledgedTask is safe
+  public Map<String, ImmutableWorkerInfo> getWorkersEligibleToRunTasks()


Looks better to be package-private if it's not supposed to be used in general.

changed, here and in few other similar places

jihoonson · 2019-12-10T23:45:39Z

+            workers,
+            input -> !lazyWorkers.containsKey(input.getKey()) &&
+                     !workersWithUnacknowledgedTask.containsKey(input.getKey()) &&
+                     !blackListedWorkers.containsKey(input.getKey()) &&


Hmm, this looks racy because each concurrent map is updated under different locks. But is this ok because this method is called periodically?

added comment to explain it.

jihoonson · 2019-12-10T23:48:48Z

-      State(int index)
+      private int index;
+      private boolean isPending;
+      private RunnerTaskState runnerTaskState;


These variables can be final.

changed, for some reason I thought enum state was immutable without even explicitly doing so.. duh

jihoonson · 2019-12-10T23:50:08Z

+    {
      if (log.isDebugEnabled()) {
        log.debug(
            new RuntimeException("Stacktrace..."),


Is this to print the stack trace so that you can see where this method was called from when the log was printed?

yes, added a comment too.

jihoonson · 2019-12-11T00:07:52Z

+             .addData("taskId", taskId)
+             .emit();
+
+          taskComplete(taskItem, null, TaskStatus.failure(taskId));


This is called outside of statusLock.

yes taskCompleted(..) has to be called outside of statusLock .. I have added comments please take a look.

Oh, sorry. I misread codes that taskComplete requires to be called under statusLock. Thanks for adding comments.

jihoonson · 2019-12-11T00:07:58Z

+
+        if (taskItem.getTask() == null) {
+          log.makeAlert("No Task obj found in TaskItem for taskID[%s]. Failed.", taskId).emit();
+          taskComplete(taskItem, null, TaskStatus.failure(taskId));


This is called outside of statusLock.

same as above

jihoonson · 2019-12-11T00:30:14Z

+
+            if (ti == null || !ti.getState().isPending()) {
+              // happens if the task was shutdown or was picked up earlier and no more pending
+              iter.remove();


Looks like pendingTaskIds is updated only here. This will lead to processing the same taskId multiple times. HRTR will assign the task to some worker in the first loop, ignore it until the worker starts processing that task in the below if clause, and then finally remove the taskId here. Can we update pendingTaskIds when the task is successfully assigned?

before leaving the lock , ti state is updated to PENDING_WORKER_ASSIGN so no other task executor thread can pick up.

pendingTaskIds variable declaration has a comment that this variable is exclusively manipulated by only external task submitter threads or task executor threads which I preferred.

Hmm, sorry I don't think I fully understand this logic yet.

before leaving the lock , ti state is updated to PENDING_WORKER_ASSIGN so no other task executor thread can pick up.

Do you mean the other threads can pick up even after the ti state is set to PENDING_WORKER_ASSIGN but will ignore it in the below if clause? Or, do they really not pick up because somehow pendingTaskIds is updated before other threads start picking up?

pendingTaskIds variable declaration has a comment that this variable is exclusively manipulated by only external task submitter threads or task executor threads which I preferred.

Yes, I think it makes sense and I prefer that too. But can pendingTaskIds be updated after this line? Like as the below code:

// set state to PENDING_WORKER_ASSIGN before releasing the lock so that this task item is not picked // up by another task execution thread. ti.setState(HttpRemoteTaskRunnerWorkItem.State.PENDING_WORKER_ASSIGN); iter.remove(); // new line to remove it from pendingTaskIds immediately taskItem = ti; break;

I don't think this is enough since the task id should be added back to pendingTaskIds if the worker couldn't execute the assigned task, but seems possible.

Do you mean the other threads can pick up even after the ti state is set to PENDING_WORKER_ASSIGN but will ignore it in the below if clause? Or, do they really not pick up because somehow pendingTaskIds is updated before other threads start picking up?

yes, other executor threads would see that but ignore because of the if clause you noted.

But can pendingTaskIds be updated after this line? Like as the below code: ...

if we removed here then we might have to add it back just in case task couldn't run for whatever reason, adding back would be complex because we will find out that we need to add it back at some later time and also adding back to its old position in the list would be difficult.
as opposed to all that we remove it from pendintTaskIds only when task is known to be no longer in PENDING or PENDING_WORKER_ASSIGN HttpRemoteTaskRunnerWorkItem.State by whichever pendingTasksExecutor thread notices that .

if you still have doubts, maybe describe a scenario where you think this doesn't work and I will try to explain how I think that scenario would play out :) .

Oh I believe this would work. I was just curious whether it was intended or not. I'm ok with it if this was intended. Would you please add a comment to avoid confusion in the future?

got it. sure, added some commentary to hopefully make it clearer.

…ttpRemoteTaskRunnerResource

… block could have NPE

himanshug · 2019-12-11T19:56:37Z

@jihoonson thanks for reviewing and you are right about LGTM alert ... I completely missed that it was used in finally block as well. moved the null check out of that try-catch .

…from submission to it getting picked up by a task execution thread

jihoonson

Thanks! It looks more clear now. +1 after CI.

jihoonson · 2019-12-12T19:17:02Z

+            // back if this thread couldn't run this task for any reason, which we will know at some later time
+            // and also we will need to add it back to its old position in the list. that becomes complex quickly.
+            // Instead we keep the PENDING_WORKER_ASSIGN to notify other task execution threads not to pick this one up.
+            // And, it is automatically removed by any of the task exeuction threads when they notice that


typo: exeuction -> execution

fixed, thanks

… not finding worker slots (apache#8697) * HRTR: make pending task execution handling to go through all tasks on not finding worker slots * make HRTR methods package private that are meant to be used only in HttpRemoteTaskRunnerResource * mark HttpRemoteTaskRunnerWorkItem.State global variables final * hrtr: move immutableWorker NULL check outside of try-catch or finally block could have NPE * add some explanatory comments * add comment on explaining mechanics around hand off of pending tasks from submission to it getting picked up by a task execution thread * fix spelling

… not finding worker slots (#8697) (#9022) * HRTR: make pending task execution handling to go through all tasks on not finding worker slots * make HRTR methods package private that are meant to be used only in HttpRemoteTaskRunnerResource * mark HttpRemoteTaskRunnerWorkItem.State global variables final * hrtr: move immutableWorker NULL check outside of try-catch or finally block could have NPE * add some explanatory comments * add comment on explaining mechanics around hand off of pending tasks from submission to it getting picked up by a task execution thread * fix spelling

himanshug added the Improvement label Oct 18, 2019

himanshug force-pushed the updates branch from 8cf89ca to dfa3214 Compare November 5, 2019 18:16

HRTR: make pending task execution handling to go through all tasks on

c8c1f40

not finding worker slots

himanshug force-pushed the updates branch from dfa3214 to c8c1f40 Compare November 6, 2019 18:27

himanshug added this to the 0.17.0 milestone Nov 19, 2019

jon-wei added the Bug label Dec 10, 2019

jihoonson reviewed Dec 11, 2019

View reviewed changes

himanshug added 5 commits December 11, 2019 10:30

Merge remote-tracking branch 'apache/master' into updates

ccc4f09

make HRTR methods package private that are meant to be used only in H…

d50e8f4

…ttpRemoteTaskRunnerResource

mark HttpRemoteTaskRunnerWorkItem.State global variables final

571e610

hrtr: move immutableWorker NULL check outside of try-catch or finally…

e20184a

… block could have NPE

add some explanatory comments

6b75c7a

himanshug added 2 commits December 12, 2019 11:04

Merge remote-tracking branch 'apache/master' into updates

dd75c9a

add comment on explaining mechanics around hand off of pending tasks …

fa4751a

…from submission to it getting picked up by a task execution thread

jihoonson approved these changes Dec 12, 2019

View reviewed changes

fix spelling

0ef2c6c

himanshug merged commit 4510118 into apache:master Dec 12, 2019

himanshug mentioned this pull request Dec 12, 2019

[Backport] HRTR: make pending task execution handling to go through all tasks on not finding worker slots (#8697) #9022

Merged

Conversation

himanshug commented Oct 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Key changed/added classes in this PR

Uh oh!

lgtm-com Bot commented Oct 18, 2019

Uh oh!

lgtm-com Bot commented Nov 6, 2019

Uh oh!

himanshug commented Dec 10, 2019

Uh oh!

jihoonson commented Dec 11, 2019

Uh oh!

jihoonson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

himanshug Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

himanshug Dec 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

himanshug Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

himanshug commented Dec 11, 2019

Uh oh!

jihoonson 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

himanshug commented Oct 18, 2019 •

edited

Loading

himanshug Dec 11, 2019 •

edited

Loading

himanshug Dec 11, 2019 •

edited

Loading

himanshug Dec 12, 2019 •

edited

Loading