Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

analyzer/block: configurable batch size #417

Merged
merged 1 commit into from
May 16, 2023

Conversation

ptrus
Copy link
Member

@ptrus ptrus commented May 15, 2023

Adds a config parameter for configuring the batch size of the block analyzer.

The default value of 100 is likely unreasonably small for production (especially if reading blocks from cache).

I think a reasonable value for production is such that the batch processing takes 30s-1min in the expected/average case (the batch processing and lock timeouts are both 5 min). Otherwise, the "mega-query" is run unnecessarily often.

Copy link
Contributor

@mitjat mitjat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

// Number of blocks to be processed in a batch.
blocksBatchSize = 100
// Default number of blocks to be processed in a batch.
defaultBatchSize = 100
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change this to 1000 or even 5000? Now that we have the cache in place (and it's prewarmed and we can copy it around with some ops busywork), it's more likely that we'll read from the cache than from the node. It's also fine if the batch is too large (for the batch timeout); it just means we'll lock some blocks too many, but the lock will auto-release soon enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to 1000.

config/config.go Outdated
Comment on lines 254 to 255
// BatchSize determines the number of blocks the block analyzer
// processes per batch.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// BatchSize determines the number of blocks the block analyzer
// processes per batch.
// BatchSize determines the maximum number of blocks the block analyzer
// processes per batch. This is relevant only when the analyzer is still catching
// up to the latest block.

@ptrus ptrus force-pushed the ptrus/feature/block-batch_size-param branch from cc9f71e to 3ee0373 Compare May 16, 2023 08:17
@ptrus ptrus force-pushed the ptrus/feature/block-batch_size-param branch from 3ee0373 to d870f41 Compare May 16, 2023 08:18
@ptrus ptrus enabled auto-merge May 16, 2023 08:19
@ptrus ptrus merged commit be96652 into main May 16, 2023
@ptrus ptrus deleted the ptrus/feature/block-batch_size-param branch May 16, 2023 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants