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

Give 404 when requesting nonexistent tasks or workers #2921

Merged
merged 1 commit into from Aug 3, 2019

Conversation

@martindurant
Copy link
Member

commented Aug 3, 2019

Fixes #2910

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Cool. Thanks for doing this @martindurant ! It's also nice to see more people get involved in the Tornado codebase.

One small thought, thoughts on responding with a more informative error message? After googling around for a bit (I didn't know how to do this) it seems like this is possible by setting the status code, then writing a normal response, and then returning (I think?)

https://www.tornadoweb.org/en/stable/web.html#tornado.web.RequestHandler.set_status
https://www.tornadoweb.org/en/stable/guide/structure.html#error-handling

No worries if you'd like to just be done though. This is clearly a nice improvement.

@martindurant

This comment has been minimized.

Copy link
Member Author

commented Aug 3, 2019

Indeed, this error is just the simplest thing, and there are many custom 40* pages, etc.; I would say it's maybe worthwhile in most cases to include an informative message, but here that message would only be "task %s not found" or somesuch, so it doesn't actually add any information.

I'll just merge this as-is, and we can consider more general error-handling on the HTTP routes another time.

@martindurant martindurant merged commit fb4e48f into dask:master Aug 3, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@martindurant martindurant deleted the martindurant:dashboard_404s branch Aug 3, 2019

@mrocklin

This comment has been minimized.

Copy link
Member

commented Aug 3, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.