-
Notifications
You must be signed in to change notification settings - Fork 409
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
pageserver: simpler, stricter config error handling #8177
Conversation
3025 tests run: 2910 passed, 0 failed, 115 skipped (full report)Code coverage* (full report)
* collected from Rust tests only The comment gets automatically updated with the latest test results
facdd2f at 2024-07-02T12:53:16.746Z :recycle: |
c21c247
to
05531c7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
general pet peeve: your error contexts / messages use present progressive tense. style guide says present tense, for maximum terseness at no information loss
Lines 200 to 247 in 4a50483
### Neon Rust code | |
#### Anyhow Context | |
When adding anyhow `context()`, use form `present-tense-verb+action`. | |
Example: | |
- Bad: `file.metadata().context("could not get file metadata")?;` | |
- Good: `file.metadata().context("get file metadata")?;` | |
#### Logging Errors | |
When logging any error `e`, use `could not {e:#}` or `failed to {e:#}`. | |
If `e` is an `anyhow` error and you want to log the backtrace that it contains, | |
use `{e:?}` instead of `{e:#}`. | |
#### Rationale | |
The `{:#}` ("alternate Display") of an `anyhow` error chain is concatenation fo the contexts, using `: `. | |
For example, the following Rust code will result in output | |
``` | |
ERROR failed to list users: load users from server: parse response: invalid json | |
``` | |
This is more concise / less noisy than what happens if you do `.context("could not ...")?` at each level, i.e.: | |
``` | |
ERROR could not list users: could not load users from server: could not parse response: invalid json | |
``` | |
```rust | |
fn main() { | |
match list_users().context("list users") else { | |
Ok(_) => ..., | |
Err(e) => tracing::error!("failed to {e:#}"), | |
} | |
} | |
fn list_users() { | |
http_get_users().context("load users from server")?; | |
} | |
fn http_get_users() { | |
let response = client....?; | |
response.parse().context("parse response")?; // fails with serde error "invalid json" | |
} | |
``` |
(We should extend that style guide to include thiserror
error messages)
I forgot a commit when merging #8177
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
I forgot a commit when merging #8177
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
I forgot a commit when merging #8177
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
I forgot a commit when merging #8177
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
I forgot a commit when merging #8177
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
I forgot a commit when merging #8177
## Problem Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment) ## Summary of changes - Make errors writing tenant config fatal (abort process) - When reading tenant config, make all I/O errors except ENOENT fatal - Replace use of bare anyhow errors with `LoadConfigError`
I forgot a commit when merging #8177
Problem
Tenant attachment has error paths for failures to write local configuration, but these types of local storage I/O errors should be considered fatal for the process. Related thread on an earlier PR that touched this code: #7947 (comment)
Summary of changes
LoadConfigError
Checklist before requesting a review
Checklist before merging