Hold certain retryable transactions until next slot#6864
Hold certain retryable transactions until next slot#6864apfitzge merged 3 commits intoanza-xyz:masterfrom
Conversation
tao-stones
left a comment
tao-stones
left a comment
There was a problem hiding this comment.
neat solution, just few minor comments.
| |(index, processing_result)| { | ||
| processing_result.was_processed().then_some(RetryableIndex { | ||
| index, | ||
| immediately_retryable: true, // recording errors are always immediately retryable |
There was a problem hiding this comment.
nit picking: instead of mapping different retryable scenarios to immediately_retryable: bool in various parts of the code, maybe consider constructing RetryableIndex with the specific reason (say enum { NotScheduled, WouldExceedCuLimit, RecordErrror, ..}) and centralizing the immediate retry decision in one place (perhaps as a member function of RetryableIndex). This could help improve clarity and maintainability.
There was a problem hiding this comment.
I think your suggestion is the more correct one, however I'd extend it. retryable_indexes should not be a thing at all...we should simply return the transaction results, and scheduler should make a decision from there.
That seems like a larger change, but I'll give it a shot.
There was a problem hiding this comment.
After looking the change is pretty involved, because retryable_indexes.len() is used in a ton of places in metrics accumulation. I think if we remove retryable indexes it should be done separately, wdyt?
There was a problem hiding this comment.
Agree that returning transaction results to scheduler is a better solution, and it should be done in separately.
| ) -> usize; | ||
|
|
||
| /// Hold the tarnsaction until the next flush (next slot). | ||
| fn hold_transaction(&mut self, priority_id: TransactionPriorityId); |
There was a problem hiding this comment.
nit: hold_transaction_for_next_slot(...)?
apfitzge
force-pushed
the
skip_transactions_until_next_slot
branch
from
0b87c3a to
10d791a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #6864 +/- ##
========================================
Coverage 82.9% 82.9%
========================================
Files 823 823
Lines 360873 360972 +99
========================================
+ Hits 299346 299462 +116
+ Misses 61527 61510 -17 🚀 New features to boost your workflow:
|
tao-stones
left a comment
tao-stones
left a comment
There was a problem hiding this comment.
PR is good as-is for the stated issue.
| |(index, processing_result)| { | ||
| processing_result.was_processed().then_some(RetryableIndex { | ||
| index, | ||
| immediately_retryable: true, // recording errors are always immediately retryable |
There was a problem hiding this comment.
Agree that returning transaction results to scheduler is a better solution, and it should be done in separately.
4 participants
Problem
Summary of Changes
Fixes #