-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Fuel/Request_Response v0.0.2: More meaningful error messages #2377
Fuel/Request_Response v0.0.2: More meaningful error messages #2377
Conversation
…l in the PeerManager table
…the request response protocol to use
This reverts commit f3b7db3b0f4aa92f2606c87ed6f65f4798734889.
…ses to unsuccessful requests
11000a8
to
7b2bf3b
Compare
7b2bf3b
to
1bc4253
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.
LGTM. I think you can run a search over : "#1311" in codebase and remvoe all todo related and make sure they are all addressed
#[error("Timeout while processing request")] | ||
Timeout = 2, | ||
#[error("Sync processor is out of capacity")] | ||
SyncProcessorOutOfCapacity = 3, |
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.
Also, maybe we need to add Unknown(u32)
for backward compatibility in the case if we can't deserialize the 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.
Tried something in 5ea9b30. There is an unknown variant, but the error code is erased away. The reason for this is that deserializing unknown values in serde has been recently implemented via the attribute serde(untagged)
, but this is not supported by postcard (fails with error WontImplement
). See serde-rs/serde#912 (comment) for the discussion on serde(untagged)
. There are other alternatives proposed, such as nested enums or custom deserialization functions for a variant, but they won't either work with postcard or are too complicated imho.
Adding an Unknown
variant which abstracts away the error code does at least ensure backwards compatibility.
Linked Issues/PRs
Closes #1311
Description
This PR follows #2362, which introduces a new version of the protocol
/fuel/req_response/0.0.2
which allows to return Errors in the response. This PR makes the following changes:ResponseSender
used by a peer to propagate received responses upstream is changed to allow the rrors to be propagatedwarning
level.Checklist
Before requesting review
After merging, notify other teams
[Add or remove entries as needed]