feat: better requeue mechanism using a separate nonce channel #643
Conversation
rach-id
added
enhancement
WalkthroughThe Go codebase has been updated to enhance the handling of nonce processing within an orchestrator. A new constant defines the queue size, and the orchestrator's start method now initializes a channel for failed nonces. The processing method has been augmented with a requeue mechanism and periodic checks, improving the system's resilience and efficiency in managing nonce failures. Changes
Poem
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 2
Configuration used: CodeRabbit UI
Files selected for processing (1)
- orchestrator/orchestrator.go (5 hunks)
Additional comments: 4
orchestrator/orchestrator.go (4)
31-35: The introduction of
queueSizealigns with the PR objectives to manage the size of the nonce queues, which is part of the improved requeue mechanism.72-78: The creation of
failedNoncesQueuewith a size defined byqueueSizeis consistent with the PR objectives to handle nonces that need to be retried.98-104: The addition of
failedNoncesQueueto theProcessNoncesmethod signature and the logic within the method to handle requeuing is in line with the PR objectives.274-279: The logic to retry processing a nonce and potentially requeue it if it fails is consistent with the PR objectives. However, the
MaybeRequeuefunction should be reviewed to ensure it adheres to the new requeue mechanism.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #643 +/- ##
==========================================
- Coverage 24.64% 24.55% -0.10%
==========================================
Files 29 29
Lines 3201 3213 +12
==========================================
Hits 789 789
- Misses 2317 2329 +12
Partials 95 95 ☔ View full report in Codecov by Sentry. |
| if len(requeueQueue) > 0 && len(noncesQueue) < queueSize { | ||
| // The use of the go routine is to avoid blocking | ||
| go func() { | ||
| nonce := <-requeueQueue |
There was a problem hiding this comment.
can we just add this as a different case in the select/for loop?
There was a problem hiding this comment.
I don't want this to be blocking, that's why I added the condition. Also, we don't want to requeue every time so that we don't keep retry indefinitely the failing nonce, but instead do it once in an hour
There was a problem hiding this comment.
mind explaining how will it block? won't it only get triggered is something is pushed through the channel to the select statement? If it will block if noncesQueue is full, then imo we should either increase that buffer or empty the channel faster. I could be missing something ofc too
There was a problem hiding this comment.
perhaps the emptying and filling of the channel should not be in the same select statement?
There was a problem hiding this comment.
If nonces queue is full and we run:
noncesQueue <- nonce
it will block, and at the same time, it won't be able to process nonces because it will be stuck here, hence the use of the go routine.
Also, we're using the ticker so that the nonsense don't get processed every time they fail directly so that we give them an hour of a delay before they get re-processed.
perhaps the emptying and filling of the channel should not be in the same select statement?
we could and it would be cleaner, but didn't want to add that extra complexity.
There was a problem hiding this comment.
why not just put the nonce in the queue when we attempt to retry instead of putting it in a channel, which eventually gets added to the nonce channel
or if we need the extra protection for some reason we can have a single goroutine that is connecting the two channels.
this could just be me being too nit picky, but spinning up a single goroutine to connect two channels on a timer feels like there's room for improvement
Overview
This PR offers a better requeue mechanism using a separate nonce channel. The reason for this is for the orchestrator to retry a nonce every hour, instead of looping instantly on it if it's up-to-date.
We could also improve this mechanism by introducing a way to reference how many tries we retired a certain before the way, but I don't think it's worth it at the moment.
Checklist
Summary by CodeRabbit
New Features
Improvements
Refactor