fix: refine error prompting #4227
Conversation
Codecov Report
@@ Coverage Diff @@
## main #4227 +/- ##
==========================================
+ Coverage 74.32% 74.33% +0.01%
==========================================
Files 844 844
Lines 122378 122336 -42
==========================================
- Hits 90956 90941 -15
+ Misses 31422 31395 -27
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
| _ => { | ||
| let bytes = { | ||
| let status = err.to_status(); | ||
| let mut bytes = Vec::<u8>::with_capacity(status.encoded_len()); | ||
| status.encode(&mut bytes).expect("Failed to encode status."); | ||
| bytes | ||
| }; | ||
| let mut header = MetadataMap::new(); | ||
| header.insert_bin(RW_ERROR_GRPC_HEADER, MetadataValue::from_bytes(&bytes)); | ||
| tonic::Status::with_metadata(Code::Internal, err.to_string(), header) | ||
| } |
There was a problem hiding this comment.
I think we should keep it, and decode in impl From<tonic::Status> for RwError, just like the legacy java fe https://github.com/singularity-data/risingwave-legacy/pull/1948/files#diff-5aee0d702428cdd3f280945a3ba0c18533c152d30427dd9b3d4d369b4e50f710R54-R65
impl From<tonic::Status> for RwError {
fn from(err: tonic::Status) -> Self {
if let Some(err) = err.metadata().get_bin(RW_ERROR_GRPC_HEADER) &&
let Ok(err) = err.to_bytes() &&
let Ok(status) = Status::decode(err) {
// still need to be improved here
return ErrorCode::InternalError(status.message).into();
}
...To be more specific, we should use Status proto stored in grpc status metadata, instead of grpc error codes. In this way, we can have more organized custom error codes and error messages in the future. cc @liurenjie1024
https://github.com/singularity-data/risingwave/blob/9d8b7d2bd7b9041f3272aed081d7cc99a36a968c/proto/common.proto#L7-L13
There was a problem hiding this comment.
(BTW, I just removed #![expect(dead_code)], so after merging main clippy will fail 🥸
There was a problem hiding this comment.
impl RwError {
fn to_status(&self) -> Status {
Status {
code: self.inner.get_code() as i32,
message: self.to_string(), // Simply discard the details and encode itself to string.
}
}Actually, we didn't encode the enum ErrorCode into a protobuf struct that can be decoded as it completely was, instead, the ErrorCode as well as its details, was only encoded as a plain string.
Hence, a ConnectorError, after being transported via grpc, if decoded strictly according to the code, will become:
connector error: connector error: xxxx
According to what we've currently implemented, it will be like internal error: connector error: xxx. So such a decoding phase won't take effect anyway.
There was a problem hiding this comment.
Yes, the current implementation doesn't make full sense, but the idea might be good in the future. Anyway I think it needs to be carefully re-designed, so I think it's also ok to remove it and KISS for now.
* fix: refine error prompting * fix clippy Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.
What's changed and what's your intention?
This PR refined some errors to be more user-friendly.
Checklist
./risedev check(or alias,./risedev c)