sweepbatcher: threshold for immediate batch publishing #1055
Conversation
Summary of ChangesHello @hieblmi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the sweep batching mechanism by introducing a new configurable threshold. When the total value of a batch of sweeps surpasses this threshold, the batch will be published without waiting for the usual initial delay, thereby accelerating the processing of high-value transactions. This ensures that significant amounts are not held up by standard batching delays, improving responsiveness for large-value operations. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a configurable threshold for immediate batch publishing in the sweepbatcher. When the cumulative value of sweeps in a batch exceeds this threshold, the batch is published immediately, bypassing the initial delay. The changes look good and are well-tested. I've suggested a small optimization to avoid redundant calculations when checking the threshold, which involves refactoring exceedsThreshold and its call site.
| func (b *batch) exceedsThreshold() bool { | ||
| if b.cfg.immediatePublishThreshold == 0 { | ||
| return false | ||
| } | ||
| return b.totalValue() >= b.cfg.immediatePublishThreshold | ||
| } |
There was a problem hiding this comment.
This function is called to check if the threshold is exceeded, and then totalValue() is called again at the call site for logging. This is inefficient as it iterates over all sweeps twice.
To optimize this, consider refactoring this function to return the total value as well, so it can be reused. This change will require updating the call site in Run.
| func (b *batch) exceedsThreshold() bool { | |
| if b.cfg.immediatePublishThreshold == 0 { | |
| return false | |
| } | |
| return b.totalValue() >= b.cfg.immediatePublishThreshold | |
| } | |
| func (b *batch) exceedsThreshold() (btcutil.Amount, bool) { | |
| if b.cfg.immediatePublishThreshold == 0 { | |
| return 0, false | |
| } | |
| total := b.totalValue() | |
| return total, total >= b.cfg.immediatePublishThreshold | |
| } |
| if b.exceedsThreshold() { | ||
| b.Infof("Batch value (%v) exceeds threshold "+ | ||
| "(%v), publishing immediately", | ||
| b.totalValue(), | ||
| b.cfg.immediatePublishThreshold) | ||
|
|
||
| // Set skipBefore to now to bypass initial | ||
| // delay. | ||
| now := clock.Now() | ||
| skipBefore = &now | ||
|
|
||
| // Trigger immediate publish by setting timer to | ||
| // fire after batchPublishDelay. | ||
| timerChan = clock.TickAfter(b.cfg.batchPublishDelay) | ||
| } |
There was a problem hiding this comment.
Following my suggestion to refactor exceedsThreshold, this block should be updated to use the new function signature. This avoids calling totalValue() twice and improves efficiency.
| if b.exceedsThreshold() { | |
| b.Infof("Batch value (%v) exceeds threshold "+ | |
| "(%v), publishing immediately", | |
| b.totalValue(), | |
| b.cfg.immediatePublishThreshold) | |
| // Set skipBefore to now to bypass initial | |
| // delay. | |
| now := clock.Now() | |
| skipBefore = &now | |
| // Trigger immediate publish by setting timer to | |
| // fire after batchPublishDelay. | |
| timerChan = clock.TickAfter(b.cfg.batchPublishDelay) | |
| } | |
| if totalVal, exceeded := b.exceedsThreshold(); exceeded { | |
| b.Infof("Batch value (%v) exceeds threshold "+ | |
| "(%v), publishing immediately", | |
| totalVal, | |
| b.cfg.immediatePublishThreshold) | |
| // Set skipBefore to now to bypass initial | |
| // delay. | |
| now := clock.Now() | |
| skipBefore = &now | |
| // Trigger immediate publish by setting timer to | |
| // fire after batchPublishDelay. | |
| timerChan = clock.TickAfter(b.cfg.batchPublishDelay) | |
| } |
starius
left a comment
starius
left a comment
There was a problem hiding this comment.
IMHO we can implement this inside initialDelayProvider. It already has totalSweptAmount argument, so it can have the threshold logic.
| func (b *batch) totalValue() btcutil.Amount { | ||
| var total btcutil.Amount | ||
| for _, sweep := range b.sweeps { | ||
| total += sweep.value | ||
| } | ||
| return total | ||
| } |
There was a problem hiding this comment.
We already have variable totalSweptAmt on the call site. We can reuse it instead of adding new method. Or we can keep the method and call it there to find the value of totalSweptAmt.
| return ok | ||
| } | ||
|
|
||
| // totalValue returns the sum of all sweep values in the batch. |
There was a problem hiding this comment.
Add a note that the method must run in the batch's event loop, so avoid races.
| } | ||
|
|
||
| // exceedsThreshold checks if the batch value exceeds the immediate publish | ||
| // threshold. Returns false if the threshold is not configured (0). |
There was a problem hiding this comment.
I propose to add the amount as an argument or to add a note that the method must be called from the batch's event loop.
| // Set skipBefore to now to bypass initial | ||
| // delay. | ||
| now := clock.Now() | ||
| skipBefore = &now | ||
|
|
||
| // Trigger immediate publish by setting timer to | ||
| // fire after batchPublishDelay. | ||
| timerChan = clock.TickAfter(b.cfg.batchPublishDelay) |
There was a problem hiding this comment.
Simplification idea.
I propose to move the whole branch above, under if initialDelay < 0 {
There we already have a couple of situations where we skip waiting. For consistency it is better to have them all in one place. And skipping can also be done by setting initialDelay = 0.
I propose also to add an empty line before delayStop := startTime.Add(initialDelay)
| assert.Contains(c, testLogger.debugMessages, stillWaitingMsg) | ||
| }, test.Timeout, eventuallyCheckFrequency) | ||
|
|
||
| // Scenario 2: Add another sweep that pushes total above threshold, |
There was a problem hiding this comment.
We also need another test case where the amount is initially below the threshold and then it increases.
| // immediatePublishThreshold is the cumulative value threshold above | ||
| // which a batch should be published immediately without waiting for | ||
| // the initial delay grouping period. If 0, the feature is disabled. | ||
| immediatePublishThreshold btcutil.Amount |
There was a problem hiding this comment.
IMHO we can implement this inside initialDelayProvider. It already has totalSweptAmount argument, so it can have the threshold logic.
2 participants
This PR Implements a configurable threshold to trigger immediate batch publishing when cumulative sweep value exceeds a specified amount, bypassing the initial delay grouping period.