-
Notifications
You must be signed in to change notification settings - Fork 1
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
Add side effect retry #21
Conversation
src/lib.rs
Outdated
.sys_run_exit(NonEmptyValue::Failure(value.into())) | ||
.sys_run_exit( | ||
RunExitResult::TerminalFailure(value.into()), | ||
RetryPolicy::Infinite, |
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.
if you change this to exponential, you enable the retry policy feature.
factor: 2.0, | ||
max_interval: None, |
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.
Why not exposing these two as well?
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.
we'll expose when we'd need them
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.
we'll need them for the e2e if you want the test to complete quickly :) https://github.com/restatedev/sdk-rust/blob/main/test-services/src/failing.rs#L122
Also I think in particular max_interval
is important, otherwise the retry delay with exponent 2 quickly evolves into some too long delay!
.map_err(Into::into) | ||
} | ||
|
||
fn sys_run_exit_failure( |
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.
Perhaps to avoid any confusion is better to rename this to sys_run_exit_failure_terminal
python/restate/context.py
Outdated
Attributes: | ||
initial_interval: The initial interval in milliseconds to wait before the first retry. | ||
max_attempts: The maximum number of attempts to retry. | ||
max_duration: The maximum duration in milliseconds to retry. | ||
""" |
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.
FYI this is the docs text I used in the other SDKs: https://github.com/restatedev/sdk-java/pull/375/files#diff-c7114c260c8e7e1653409b43f92d24cfd46e51517bf2ed166871dc963de54ff0R16
except: | ||
# The VM decided to retry, therefore we tear down the current execution | ||
return None |
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.
Why not rethrowing (as you were doing before essentially)?
4cb81f5
to
b878470
Compare
No description provided.