274 Improve handling of issue request execution #276
Conversation
b-yap
changed the title
clients/wallet/src/task.rs
Outdated
| pub struct LedgerTask { | ||
| ledger: Ledger, |
There was a problem hiding this comment.
I think this implementation is so general that in theory, we could generify it so that it is not specific to just 'ledgers' but to anything. You implemented some struct that has some kind of ID (the ledger field) and is able to hold info about its current status, and can retry some number of times.
If we generify the ID passing we could also use this in other contexts. Counter-argument would be that we over-engineer this while we don't need to, but since it's little extra work I would propose we add those changes too. WDYT @b-yap ?
There was a problem hiding this comment.
In any case, I would like to rename this. The name would be different if we generify, however.
There was a problem hiding this comment.
I just changed it to SlotTask because there's no alternate name provided. Do you have any suggestions (even just a temp one would be fine ✌️)?
And yes, creating a generic out of this struct did cross my mind, but I cannot find yet on applying it elsewhere.
Should we create a ticket and explore the "possibilities" from there? 👏 😀
There was a problem hiding this comment.
Hmm yeah I honestly don't have a way better name ready for that. Something with 'Observable...Task' could be nice but it's not an observable in the classic sense so also a bit misleading.
I'm fine with keeping that name for now. And you're right, let's only extend the task struct once we found the need to use something similar elsewhere. No need to create a ticket for that I suppose :)
clients/vault/src/issue.rs
Outdated
| // An existing task is found. | ||
| if let Some(existing) = processed_map.get_mut(ledger) { | ||
| // Only recoverable errors can be given a new task. | ||
| return existing.recover_with_new_sender() | ||
| } | ||
|
|
||
| // Not finding the ledger in the map means there's no existing task for this ledger. | ||
| let (sender, receiver) = tokio::sync::oneshot::channel(); | ||
| tracing::trace!("Creating a task for ledger {}", ledger); | ||
|
|
||
| let ledger_task = SlotTask::new(*ledger, receiver); | ||
| processed_map.insert(*ledger, ledger_task); | ||
|
|
||
| Some(sender) |
There was a problem hiding this comment.
this is now a bit inconsistent with 'ledger' vs 'slot'
you should probably update the comment and variable name too 😅
b-yap
force-pushed
the
274-improve-handling-of-issue-request-execution
branch
from
a9c15ec to
df92684
Compare
b-yap
force-pushed
the
274-improve-handling-of-issue-request-execution
branch
from
bd01689 to
e63b158
Compare
…case for the task.
b-yap
force-pushed
the
274-improve-handling-of-issue-request-execution
branch
from
e86585d to
4d77bfe
Compare
This reverts commit 4d77bfe.
|
I ran the tests locally and they work, so I'm merging this |
I introduced 1 new structure and enum:
LedgerTaskandLedgerTaskStatusrespectively.These should be sufficient enough to make sure that a new task is only run IF and only IF the task failed, and this failure is recoverable.