Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix worker tool box in visualizer #2063

Merged
merged 2 commits into from
Mar 14, 2017
Merged

Conversation

riga
Copy link
Contributor

@riga riga commented Mar 14, 2017

Description

The worker toolbox in the visualizer shows some buggy information in the phase between adding a worker to the scheduler and actual task processing (so while the dependency tree is built). This looks like:

bug

The reason is that workers is both used for the list of worker objects in the visualizer for rendering and the number of worker processes of a worker. See here (first and last line in the marked area). Initially, there's no 'worker' field in the worker object so Mustache falls back to the list of workers which renders to the example above.

Motivation and Context

A simple renaming of the variable that holds the list of worker objects for rendering fixes the bug (2nd commit). In addition, the worker toolbox is removed now entirely when a worker is disabled (1st commit). Previously, the "disable" button was removed while the "add workers" menu was still visible.

@mention-bot
Copy link

@riga, thanks for your PR! By analyzing the history of the files in this pull request, we identified @daveFNbuck, @nmb10 and @Tarrasch to be potential reviewers.

@Tarrasch
Copy link
Contributor

How was this tested by the way? Looks good too me.

@Tarrasch Tarrasch merged commit 3442304 into spotify:master Mar 14, 2017
@Tarrasch
Copy link
Contributor

Whoups, I wanted to use the rebase merge strategy. Oh well. Not too big deal.

@riga
Copy link
Contributor Author

riga commented Mar 14, 2017

I created a short test task that just did a time.sleep(n) to delay the dependency tree building. Wasn't sure how this could be covered by a test case. Thanks for merging this so fast.

@riga riga deleted the fixWorkerTools branch March 14, 2017 16:44
This was referenced Jun 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants