Remove optimization of not sending small batches #50
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
This stack of pull requests is managed by Graphite. Learn more about stacking. |
vegarsti
changed the title
vegarsti
force-pushed
the
send-tiny-batches
branch
from
55e8dae to
8f35579
Compare
adammilnesmith
approved these changes
Jul 1, 2024
There was a problem hiding this comment.
100% on the money - LGTM. We will send every 500ms by default if there's at least one block now (as trySendBlockBatch() has a check already to not send when the batch is completely empty). Looking at some of this code raised a couple of other questions for me not related to this change so will follow up with those separately.
msf
reviewed
Jul 18, 2024
| @@ -70,10 +70,6 @@ func (i *ingester) trySendCompletedBlocks( | |||
| nextBlockToSend int64, | |||
| ) (int64, error) { | |||
| for { | |||
| if len(collectedBlocks) < maxBatchSize/10 { | |||
There was a problem hiding this comment.
this impacts throughput on catchup scenarios, but yes, lets remove and validate perf on next large "catchup".
my testing showed we would average ±50% of the maxBatchSize without this, by flip-flopping between very small batches and full batches
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Conduit reported "high latency". It might be due to this.
We're currently not sending batches of <10 blocks (1/10th of the max batch size of 100), to optimize throughput. When the indexer is caught up, this leads to (imo) unnecessarily high latency, as BOB produces blocks every 300ms and we'll thus only send a request every 3 seconds. We should instead be sending as often as possible, or at least the default of every half a second.
One option is to have some sort of "am I backfilling or not" flag. I suggest just removing this optimization for now, and rather revisit this if/when we see that the next time we're backfilling, we're unnecessarily sending many small batches.
I'm fairly convinced that when backfilling, this optimization ~never triggers anyway, since the requests to the node should be filling up the buffer of blocks to send.
@adammilnesmith let's discuss this one.