-
Notifications
You must be signed in to change notification settings - Fork 94
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
[r2r] Repeatable future #1564
[r2r] Repeatable future #1564
Conversation
* Use the `repeatable` future as an example at `utxo_coin_builder.rs`
* Refactor `custom_futures` * Remove `SendAll` future
* Add `Repeatable::inspect_err` * Add the possibility to convert `Result<T, E>` into `Action<T, E>`
* Add `retry_on_err` macro that expects from the future to return a result
* Fill doc comments
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.
Thanks a lot for this very useful feature. Just one question and a small change :)
@shamardy @borngraced @laruh @caglaryucekaya, PR is ready for the review :) |
while now_ms() / 1000 < wait_until { | ||
Timer::sleep(3.).await; | ||
// Let the storage to be initialized for the given coin. | ||
Timer::sleep(1.).await; |
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.
you changed the time?? why tho?
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've left the sleep timeout between attempts the same - 3 seconds as before:
.repeat_every(Duration::from_secs(3))
But as you can see (before my changes), the sleep timer was at the beginning of this function (i.e. before the first attempt). It's required to let the storage to initialize everything it needs before the first my_tx_history_v2_impl
attempt.
repeatable
doesn't allow to sleep before the first attempt, so I decided to put a sleep at the beginning of this function.
It's enough to sleep for 1 second to let the storage to initialize all required SQL tables.
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.
gotcha 👌🏼
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.
GREAT! looks very good.
just a thought and question.
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.
Awesome 🔥
I have just small suggestions :)
d7066ec
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.
🔥
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.
One question, but it's a major one 🙂
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.
Next review iteration 🙂
* Allow the user to specify only `until_ms` * Avoid declaring `exec_fut: F` and `repeat_every: Duration` optional
* Add `RepeatUntil` enum * Set the default `repeat_every` to 1s * Panic on `total_attempts == 0`
@artemii235 the PR is ready for the next review |
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.
Last note 🙂
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.
🔥
No description provided.