s3: Improve error messages and handling #10125
Conversation
* Include more information in all error messages so that users can understand what caused the problem * ListBucketsError is a fatal error, but we were just sending two messages instead of making it a hard error.
quodlibetor
force-pushed
the
s3-detailed-error-messages
branch
from
fa1be10 to
5f11c57
Compare
| err, | ||
| })) | ||
| .await | ||
| .unwrap_or_else(|e| tracing::debug!("Source queue has been shut down: {}", e)); |
There was a problem hiding this comment.
does this only happen during shutdown?
| IoError { | ||
| bucket: String, | ||
| err: std::io::Error, | ||
| }, | ||
| } | ||
|
|
||
| impl std::error::Error for S3Error {} |
There was a problem hiding this comment.
I think it would be nice to implement source on the error impl, for the cases that have inner errors (which right now is all of them), maybe a good TODO?
There was a problem hiding this comment.
Yeah, currently we print all causes in the display impl instead of implementing the causes method. There is a rust-general discussion about what the best thing to is, but this is generally what we do in mz.
For now I think it's more confusing to both impl display of causes and the cause() method, but I do think it would be better in general if we impl'd cause() everywhere and used anyhow's {:#?} format for final display of errors.
| S3Error::GetObjectError { .. } | S3Error::ListObjectsFailed(_) => { | ||
| Ok(NextMessage::Pending) | ||
| } | ||
| Some(Some(Err(e))) => match e { |
There was a problem hiding this comment.
what does the double-Option mean? another good TODO to change that to an explicit enum
There was a problem hiding this comment.
I believe that the double-option is an an artifact of using now_or_never() on an async queue -- recv() yields an Option, and now_or_never() yields an Option of the thing it's called on.
| Some(Some(Err(e))) => match e { | ||
| S3Error::GetObjectError { .. } => { | ||
| tracing::warn!( | ||
| "when reading source '{}' ({}): {}", |
There was a problem hiding this comment.
| "when reading source '{}' ({}): {}", | |
| "error when reading source '{}' ({}): {}", |
There was a problem hiding this comment.
I don't think that adding error to warn! logs is helpful, it just makes grep error (and especially case-insensitive search in e.g. less) less useful/more confusing. This will currently show up as WARN <mod> when reading source....
I agree that this is a sentence fragment, though.
Yes, I think that that's the original motivation. I'm not 100% convinced that it's the right decision, but right now there's no way to get a dataflow out of an error state, so I'm hesitant to error one just because any given single item was missing. If we implemented MaterializeInc/database-issues#2060 it would be possible for people to be more precise about what they want to ingest than currently, which would make it more reasonable to error on anything invalid. |
what caused the problem
instead of making it a hard error.
Motivation
This PR fixes a previously unreported bug:
We were providing useless error messages for S3 errors.
Checklist