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

Speed up task_list when beyond limit #2239

Merged
merged 1 commit into from
Oct 24, 2017

Conversation

daveFNbuck
Copy link
Contributor

@daveFNbuck daveFNbuck commented Sep 26, 2017

Description

In task_list, just return the count for the status when that count is over the max_shown_tasks limit without iterating to check upstream status. This means we can't show counts for upstream statuses, so those are replaced with "unknown" in the visualizer when this happens.

Motivation and Context

When the number of tasks get into the millions, even refreshing the visualizer can take a minute or more, causing havoc in the pipeline. Since all we really want in these situations is the counts, we can skip the more expensive bits of computation and just return the sizes. This prevents doing upstream checks, but saves a lot of time.

We may want to institute a higher threshold so we can get upstream numbers if you're only a little above the limit for returning all tasks.

Have you tested this? If so, how?

Added unit tests, have been running this for about a day and it works at scale to reduce visualizer refreshes from minutes to seconds.

When the number of tasks get into the millions, even refreshing the
visualizer can take a minute or more, causing havoc in the pipeline.
Since all we really want in these situations is the counts, we can skip
the more expensive bits of computation and just return the sizes. This
prevents doing upstream checks, but saves a lot of time.

We may want to institute a higher threshold so we can get upstream
numbers if you're only a little above the limit for returning all
tasks.
Copy link
Collaborator

@dlstadther dlstadther left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Contributor

@Tarrasch Tarrasch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dlstadther dlstadther merged commit 103136d into spotify:master Oct 24, 2017
@dlstadther
Copy link
Collaborator

Thanks @daveFNbuck !

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