Block-builder-scheduler initial structure #9650
Open
+208
−19
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
pracucci
approved these changes
Oct 21, 2024
| } | ||
|
|
||
| func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | ||
| f.StringVar(&cfg.BuilderConsumerGroup, "block-builder-scheduler.builder-consumer-group", "block-builder", "The Kafka consumer group used by block-builders.") |
There was a problem hiding this comment.
Not sure what this will be used for. I didn't expect block builders to have a consumer group at all, given all offsets should be tracked by the scheduler. I suggest to remove this for now, and re-introduce it in a later PR if it will be effectively used (if so, why?).
There was a problem hiding this comment.
Ah yes. This is something @codesome and I discussed: in the v1 of the scheduler, we'll just have the block-builder workers continue to checkpoint offsets to Kafka. For 2 reasons:
- get v1 out quicker
- less risky for backlogs: if there is a large backlog that takes 6 hours to consume, we'll get smaller section-granularity checkpoints rather than everything riding on that 6 hour job to complete.
(But it's mostly about reason number 1.) Let me know your thoughts.
|
|
||
| func (cfg *Config) RegisterFlags(f *flag.FlagSet) { | ||
| f.StringVar(&cfg.BuilderConsumerGroup, "block-builder-scheduler.builder-consumer-group", "block-builder", "The Kafka consumer group used by block-builders.") | ||
| f.StringVar(&cfg.SchedulerConsumerGroup, "block-builder-scheduler.scheduler-consumer-group", "block-builder-scheduler", "The Kafka consumer group used by block-builder-scheduler.") |
There was a problem hiding this comment.
Consider removing scheduler- prefix if ends up we'll not need the builder consumer groups too.
Suggested change
| f.StringVar(&cfg.SchedulerConsumerGroup, "block-builder-scheduler.scheduler-consumer-group", "block-builder-scheduler", "The Kafka consumer group used by block-builder-scheduler.") | |
| f.StringVar(&cfg.SchedulerConsumerGroup, "block-builder-scheduler.consumer-group", "block-builder-scheduler", "The Kafka consumer group used by block-builder-scheduler.") |
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.
What this PR does
Adds initial structure for
block-builder-schedulerliving at pkg/blockbuilder/scheduler. Adds a newblock-builder-schedulertarget to the binary. This target currently just connects to Kafka and records per-partition start and end offset gauge metrics. In the future it will compute jobs and assign them to block-builder workers.Checklist
CHANGELOG.mdupdated - the order of entries should be[CHANGE],[FEATURE],[ENHANCEMENT],[BUGFIX].about-versioning.mdupdated with experimental features.