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

Fixes a race in the scheduling limits. #3417

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

cyriltovena
Copy link
Contributor

Found out the hard way that sharding might not listen correctly to context and so still run queries
after the upstream request ends. This can lead to panic when scheduling work which send on a closed channel.

I've added a safeguard to wait for all jobs to end before ending the original request but also attempted to fix the sharding downstreamer
to properly stop sending work when context is closed.

Signed-off-by: Cyril Tovena cyril.tovena@gmail.com

Special notes for your reviewer:

Checklist

  • Documentation added
  • Tests updated

Found out the hard way that sharding might not listen correctly to context and so still run queries
after the upstream request ends. This can lead to panic when scheduling work which send on a closed channel.

I've added a safe guard to wait for all jobs to end before ending the original request but also attemped to fix the sharding downstreamer
to properly stop sending work when context is closed.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
@cyriltovena
Copy link
Contributor Author

/cc @dannykopping we found this together.
/cc @slim-bean seems like a recut of k43 sorry.

Signed-off-by: Cyril Tovena <cyril.tovena@gmail.com>
Copy link
Contributor

@dannykopping dannykopping 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
Collaborator

@slim-bean slim-bean 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

@kavirajk kavirajk left a comment

Choose a reason for hiding this comment

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

LGTM!

@cyriltovena cyriltovena merged commit 20ef66d into grafana:master Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants