Scheduler: don't give tasks to disabled assistants #1839

Tarrasch · 2016-09-01T07:30:34Z

Description

Previously assistants would keep getting tasks and never stop. Despite
that a user have clicked "disable worker" in the WebUI.

Motivation and Context

This is a bugfix.

Have you tested this? If so, how?

I have included unit tests. I have yet to test in production environment.

Tarrasch · 2016-09-06T03:56:26Z

@daveFNbuck, as the founder of assistants feature. Do you mind reviewing this?

daveFNbuck · 2016-09-06T06:32:07Z

Won't we need to add worker logic to kill the assistant after this? It's not ideal if the assistant process stays permanently alive after this.

Tarrasch · 2016-09-06T07:27:29Z

Oh. I assumed assistants would break their get_work loop once the scheduler says there's no more tasks. Let me double check this.

Tarrasch · 2016-09-12T11:17:44Z

This PR is yet not done although still sensible to review. I need to add tests that actually disable a worker. The tests I've touched upon so far are about sending interrupts to the worker (only vaguely related).

Tarrasch · 2016-09-13T04:46:47Z

Feels a bit scary adding another thing to get_work. But does it make sense to you @daveFNbuck?

daveFNbuck · 2016-09-13T05:37:59Z

Looks good, but it's missing a way to tell the viz about whether the worker is disabled. You can't disable a worker if it doesn't have any pending jobs without that.

Tarrasch · 2016-09-13T07:05:02Z

Really? I thought the visualizer already had this information in full. Because after clicking on the fire extinguisher button, the worker "bar" is greyed out. Eh,, I'll let pics explain this:

before click:

after click:

daveFNbuck · 2016-09-13T19:30:00Z

It uses whether the worker has any pending tasks to determine whether to
gray the bar out. If you have an assistant with no pending tasks, the bar
is already grayed out and you won't have the button to kill it.

On Tue, Sep 13, 2016 at 12:05 AM, Arash Rouhani notifications@github.com
wrote:

Really? I thought the visualizer already had this information in full.
Because after clicking on the fire extinguisher button, the worker "bar" is
greyed out. Eh,, I'll let pics explain this:

before click:

[image: selection_045]
https://cloud.githubusercontent.com/assets/294349/18464398/02854994-79bb-11e6-9a59-81c6f78b4a59.png

after click:

[image: selection_046]
https://cloud.githubusercontent.com/assets/294349/18464399/067bfde0-79bb-11e6-96e4-279ad3abaa74.png

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1839 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB_rbcRU_tmkPK04AlcVw_xOA06L4SM8ks5qpksqgaJpZM4JyZTj
.

Tarrasch · 2016-09-14T04:34:17Z

Oh, put your safety belt on everyone, I'm going to mess around in frontend land then. ^^

Thanks for the clarification @daveFNbuck, I'll try to see where I can get with this.

daveFNbuck · 2016-09-14T14:38:54Z

I can do it if this part is too unfamiliar for you.

On Tue, Sep 13, 2016 at 9:34 PM, Arash Rouhani notifications@github.com
wrote:

Oh, put your safety belt on everyone, I'm going to mess around in frontend
land then. ^^

Thanks for the clarification @daveFNbuck https://github.com/daveFNbuck,
I'll try to see where I can get with this.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1839 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB_rbc_op65sV2LI2mUKS3Q1DP7ZtOQDks5qp3lXgaJpZM4JyZTj
.

Tarrasch · 2016-09-15T02:03:43Z

I'll take a stab at it today. If I fail miserably I'll see how we can cooperate :)

Tarrasch · 2016-09-16T04:05:26Z

@daveFNbuck, I've got stuck on the .html templating part. I initially thought we use tornado for html templating (here and here. But it seems like no.

I'm editing luigi/static/visualiser/index.html and I don't see syntax like {% if blah%} but instead it uses {{#var_name}} which doesn't seem to be Tornado's thingie. Confusingly, the html file luigi/templates/layout.html seem to use the tornado syntax.

Can you say what template engine provides {{#var_name}} syntax so I can google for docs?

daveFNbuck · 2016-09-16T05:48:54Z

I also don't know the answer on that one. I've mostly just tweaked around the edges on what was already there.

Tarrasch · 2016-09-16T08:37:52Z

@freider @erikbern @stephenpascoe, quick question. Do you know what HTML templating syntax luigi's visualiser uses? See the comment above.

Tarrasch · 2016-09-16T10:02:08Z

Ah, luigi is using Mustache as of #80. I'm not sure about what version and stuff though. Triggered by this line of code.

Tarrasch · 2016-09-16T10:51:08Z

Oh boy am I excited after my first line of written javascript!

Mostly unrelated: Kudos to @fabriziodemaria and everyone involved in the recent improvements to the execution summary and return codes. The summary prints exactly what one expects after disabling the worker:

Previously assistants would keep getting tasks and never stop. Despite that a user have clicked "disable worker" in the WebUI.

This is so we scan make the WebUI aware of the state. (upcoming commit)

Previously it was equivalent to "has no pending tasks". But because we also associate "greyed out" with "can't click disable", the WebUI have confused at least me for a while. This is also useful because you can now disable the worker while it's still in the scheduling phase.

Tarrasch · 2016-09-20T02:04:16Z

@daveFNbuck, I would appreciate a final round of review. :)

daveFNbuck

LGTM

daveFNbuck · 2016-09-20T02:19:39Z

luigi/static/visualiser/index.html

@@ -308,6 +311,9 @@ <h3 class="box-title">{{name}}</h3>
                    Running: {{num_running}}<br>
                    Pending: {{num_pending}}<br>
                    Unique Pending: {{num_uniques}}<br>
+                    {{#is_disabled}}
+                    This worker is <b>disabled</b>. It will not start on new tasks.<br>


It will not start new tasks. No "on"

Tarrasch mentioned this pull request Sep 1, 2016

tests: Skip a inttermittently failing sqla test #1840

Merged

Tarrasch mentioned this pull request Sep 13, 2016

worker_tests.py: Fix minor resource leak #1853

Merged

Tarrasch force-pushed the dont_give_tasks_to_disabled_assistants branch from 196f5d3 to 2d06f76 Compare September 13, 2016 03:13

Tarrasch force-pushed the dont_give_tasks_to_disabled_assistants branch from 2aaa07d to d225121 Compare September 13, 2016 04:48

Tarrasch added 6 commits September 19, 2016 09:06

Scheduler: don't give tasks to disabled assistants

a3c4b9b

Previously assistants would keep getting tasks and never stop. Despite that a user have clicked "disable worker" in the WebUI.

Let get_work reply if a workes is disabled

ac726ab

Pass along the worker-state in worker_list

cac15e6

This is so we scan make the WebUI aware of the state. (upcoming commit)

Fix case where execution summary would crash

439d2a2

WebUI: describe the effects of disabling a task

0886a23

Tarrasch force-pushed the dont_give_tasks_to_disabled_assistants branch from 24a1bc6 to 0886a23 Compare September 19, 2016 02:29

daveFNbuck reviewed Sep 20, 2016

View reviewed changes

index.html: Spelling error fix

0f1b94e

Tarrasch merged commit cac500a into spotify:master Sep 20, 2016

Tarrasch deleted the dont_give_tasks_to_disabled_assistants branch September 20, 2016 09:27

This was referenced Jun 29, 2022

no mo enum 34 #3180

Closed

enum34 be gone #3181

Closed

mdragilev mentioned this pull request Jun 28, 2024

for S3 contrib package move to boto3 Affirm/luigi#26

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scheduler: don't give tasks to disabled assistants #1839

Scheduler: don't give tasks to disabled assistants #1839

Tarrasch commented Sep 1, 2016

Tarrasch commented Sep 6, 2016

daveFNbuck commented Sep 6, 2016

Tarrasch commented Sep 6, 2016

Tarrasch commented Sep 12, 2016

Tarrasch commented Sep 13, 2016

daveFNbuck commented Sep 13, 2016

Tarrasch commented Sep 13, 2016

daveFNbuck commented Sep 13, 2016

Tarrasch commented Sep 14, 2016

daveFNbuck commented Sep 14, 2016

Tarrasch commented Sep 15, 2016

Tarrasch commented Sep 16, 2016

daveFNbuck commented Sep 16, 2016

Tarrasch commented Sep 16, 2016

Tarrasch commented Sep 16, 2016 •

edited

Loading

Tarrasch commented Sep 16, 2016

Tarrasch commented Sep 20, 2016

daveFNbuck left a comment

daveFNbuck Sep 20, 2016

Scheduler: don't give tasks to disabled assistants #1839

Scheduler: don't give tasks to disabled assistants #1839

Conversation

Tarrasch commented Sep 1, 2016

Description

Motivation and Context

Have you tested this? If so, how?

Tarrasch commented Sep 6, 2016

daveFNbuck commented Sep 6, 2016

Tarrasch commented Sep 6, 2016

Tarrasch commented Sep 12, 2016

Tarrasch commented Sep 13, 2016

daveFNbuck commented Sep 13, 2016

Tarrasch commented Sep 13, 2016

daveFNbuck commented Sep 13, 2016

Tarrasch commented Sep 14, 2016

daveFNbuck commented Sep 14, 2016

Tarrasch commented Sep 15, 2016

Tarrasch commented Sep 16, 2016

daveFNbuck commented Sep 16, 2016

Tarrasch commented Sep 16, 2016

Tarrasch commented Sep 16, 2016 • edited Loading

Tarrasch commented Sep 16, 2016

Tarrasch commented Sep 20, 2016

daveFNbuck left a comment

Choose a reason for hiding this comment

daveFNbuck Sep 20, 2016

Choose a reason for hiding this comment

Tarrasch commented Sep 16, 2016 •

edited

Loading