Don't log details and metadata when logging Status error #3015
Conversation
There was a problem hiding this comment.
Thanks a lot for creating this PR @zhaohaidao. The solution to introduce a new type looks really good to me :-) I left a few suggestions how we could extend it a little bit more. Once this is done, I believe that we can merge this PR.
crates/core/src/protobuf.rs
Outdated
| @@ -166,7 +167,7 @@ fn to_read_err(status: Status) -> ReadError { | |||
|
|
|||
| fn to_write_err(status: Status) -> WriteError { | |||
| match status.code() { | |||
| Code::FailedPrecondition => WriteError::FailedPrecondition(status.to_string()), | |||
| Code::FailedPrecondition => WriteError::FailedPrecondition(SimpleStatus::from(status).to_string()), | |||
There was a problem hiding this comment.
Can we use SimpleStatus for the ReadError above as well? We probably need to let SimpleStatus implement Error to make it work. There are also a few other places where it could be beneficial (e.g. NodeOperationError or the StatusError).
@tillrohrmann Hi, all comments have been addressed. PLAT |
There was a problem hiding this comment.
Thanks a lot for updating the PR @zhaohaidao. It looks really good. I left one minor comment. If you can address this comment and rebase onto the latest main, then I'll be able to merge this PR :-)
@tillrohrmann comments are addressed. PTAL |
There was a problem hiding this comment.
Thanks a lot for fixing this issue @zhaohaidao. This is a really good improvement. The changes look good to me :-) Merging it once GHA gives green light.
to resolve #2607