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

tune loglevel of messages in depot and handle db full errors #400

Merged
merged 1 commit into from
Apr 19, 2016

Conversation

reset
Copy link
Collaborator

@reset reset commented Apr 18, 2016

This will set the log level of messages to an appropriate level for running this service in production. I also have fixed one of the TODOs to handle database full messages on upload

gif-keyboard-17034412112207007438

@chef-delivery
Copy link
Contributor

This PR has passed 'Verify' and is ready for review and approval!
Use: '@delivery approve' when code review is complete.

error!("Database full, err={:?}", e);
return Ok(Response::with(status::InsufficientStorage));
}
Err(e) => panic!("Unhandled database error on upload, err={:?}", e),
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably return an Ok(Response::with(status::InternalServerError)) rather than a panic, otherwise the client side may just get a dropped connection when the request thread panics.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The router's chain will always return a 500 to the client if you panic in a handler

@reset
Copy link
Collaborator Author

reset commented Apr 19, 2016

@delivery approve

@chef-delivery chef-delivery merged commit 2ab0757 into master Apr 19, 2016
@chef-delivery
Copy link
Contributor

Change: 76944206-d034-4e9e-8447-f6b346b0719b approved by: @reset

@chef-delivery chef-delivery deleted the logging-depot branch April 19, 2016 17:40
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