-
Notifications
You must be signed in to change notification settings - Fork 210
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
refactor: client error handling #4064
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4064 +/- ##
==========================================
- Coverage 58.34% 58.31% -0.04%
==========================================
Files 194 194
Lines 42618 42644 +26
==========================================
+ Hits 24865 24867 +2
- Misses 17753 17777 +24 ☔ View full report in Codecov by Sentry. |
33cad7b
to
a984553
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.
I think this is a good direction to go. We'll need to build tooling to either detect zombie state machines or always include timeout state transitions to not overload the executor, but that sounds doable.
I'll ACK to reduce latency, but please take a look at my one comment.
e82f103
to
10b0508
Compare
After thinking about fedimint#4051 and discussion inside it, it seems to me that `is_retryable` is just a terrible pattern, so in this change: * get rid of `is_retryable` * make all federation errors retryable * add function to report only errors that is somewhat important * go over callsites and do something (hopefully) better
10b0508
to
7ad74ab
Compare
rebased |
After thinking about #4051 and discussion inside it, it seems to me that
is_retryable
is just a terrible pattern, so in this change:is_retryable