-
Notifications
You must be signed in to change notification settings - Fork 599
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
chore: refine error msg of aws sdk #11288
Conversation
Codecov Report
@@ Coverage Diff @@
## main #11288 +/- ##
==========================================
- Coverage 69.77% 69.77% -0.01%
==========================================
Files 1359 1359
Lines 225184 225178 -6
==========================================
- Hits 157125 157118 -7
- Misses 68059 68060 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/meta/src/hummock/manager/mod.rs
Outdated
Ok(metadata) => metadata, | ||
Err(_) => { | ||
return Err(ObjectError::internal( | ||
"Fail to access remote object storage, |
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.
Here we want to see this error message every time meta fails to access object storage, and not only aws s3, but also gcs, oss...
So maybe we can not limite it to only aws error.
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.
It's not limited to AWS error. Here we just propagate the error, so the message is completely decided by the error itself.
The problem is AWS error's Display
, which is fixed by this PR.
e.g., this is the panic msg of gcs (opendal)
thread 'risingwave-main' panicked at 'called `Result::unwrap()` on an `Err` value: ObjectStore(Unexpected (temporary) at Pager::next, context: { called: reqsign::LoadCredential, service: gcs, path: hummock_001/cluster_id/ } => loading credential to sign http request, source: error sending request for url (http://metadata.google.internal/computeMetadata/v1/instance/service-accounts/default/token?scopes=https://www.googleapis.com/auth/devstorage.read_write): error trying to connect: dns error: failed to lookup address information: nodename nor servname provided, or not known
backtrace of `ObjectError`:
BTW, opendal even has other logging
2023-07-28T12:56:18.140486+02:00 ERROR opendal::services: service=gcs operation=list path=hummock_001/cluster_id/ -> failed: Unexpected (temporary) at Pager::next => loading credential to sign http request
Context:
called: reqsign::LoadCredential
service: gcs
path: hummock_001/cluster_id/
Source: error sending request for url (http://metadata.google.internal/computeMetadata/v1/instance/service-accounts/default/token?scopes=https://www.googleapis.com/auth/devstorage.read_write): error trying to connect: dns error: failed to lookup address information: nodename nor servname provided, or not known
Caused by:
0: error trying to connect: dns error: failed to lookup address information: nodename nor servname provided, or not known
1: dns error: failed to lookup address information: nodename nor servname provided, or not known
2: failed to lookup address information: nodename nor servname provided, or not known
Stack backtrace:
0: std::backtrace_rs::backtrace::libunwind::trace
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.
I'm also comfortable to add more error message here, or panic directly. But we should at least also provide the original error message, which turns out to be very informative.
And personally I think "please check if your Access Key and Secret Key are configured correctly" is not always true and can be misleading, so I removed it.
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.
LGTM. Thanks.
I hereby agree to the terms of the RisingWave Labs, Inc. Contributor License Agreement.
What's changed and what's your intention?
#10993
example: (It's a little verbose and contains duplicated information, but at least provides the context)
Previously it's only
ObjectStore(service error
, #11198 improved a little bit (I didn't notice it), but I think this PR is more informative.Checklist
./risedev check
(or alias,./risedev c
)Documentation
Click here for Documentation
Types of user-facing changes
Please keep the types that apply to your changes, and remove the others.
Release note