ingest-storage: support multiple kafka seed broker addresses#14328
ingest-storage: support multiple kafka seed broker addresses#14328srpvpn wants to merge 7 commits intografana:mainfrom
Conversation
56quarters
left a comment
There was a problem hiding this comment.
Thanks for the PR! I'm not super familiar with ingest storage so I'll let someone from the ingest squad take a look.
pkg/storage/ingest/config.go
Outdated
| cfg.concurrentFetchersFetchBackoffConfig = defaultFetchBackoffConfig | ||
|
|
||
| f.StringVar(&cfg.Address, prefix+"address", "", "The Kafka backend address.") | ||
| f.StringVar(&cfg.Address, prefix+"address", "", "The Kafka seed broker address, or a comma-separated list of seed broker addresses.") |
There was a problem hiding this comment.
There's a type in dskit called StringSliceCSV that handles this. There's an example here.
There was a problem hiding this comment.
I think, the comment above was about using the flagext.StringSliceCSV for cfg.Address. Any reason we can't do that here?
There was a problem hiding this comment.
Okay: KafkaConfig.Address now uses flagext.StringSliceCSV directly, and the flag is registered via f.Var(&cfg.Address, ...). SeedBrokers still keeps trimming and
backward-compatible fallback behavior. Also updated ingest tests/fixtures to set address through the same CSV parsing path.
tacole02
left a comment
There was a problem hiding this comment.
Docs look good! Thank you!
narqo
left a comment
There was a problem hiding this comment.
Thank you for iterating on that. Please see the comments. After addressing those, please check the tests. I think there are more places around Mimir, that use KafkaConfig.Address. Those tests need to be fixed.
pkg/storage/ingest/config_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestKafkaConfig_AddressCSV(t *testing.T) { |
There was a problem hiding this comment.
This test is redundant and doesn't bring value: it checks what must be guarantied by the type used. Please, cut.
pkg/storage/ingest/util_test.go
Outdated
| require.NoError(t, CreateTopic(cfg, logger)) | ||
| }) | ||
|
|
||
| t.Run("should support multiple comma-separated seed broker addresses", func(t *testing.T) { |
There was a problem hiding this comment.
Can you explain what this subtest has to do with TestCreateTopic — does passing multiple seed addresses change how a topic is created?
There was a problem hiding this comment.
Removed the redundant TestKafkaConfig_AddressCSV and dropped the unrelated multi-seed subtest from TestCreateTopic.
Judging by the broken CI, this comment still needs to be address. |
|
@narqo Addressed. I fixed the remaining test usages of |
2279b09 to
8009b06
Compare
| WriteTimeout time.Duration `yaml:"write_timeout"` | ||
| WriteClients int `yaml:"write_clients"` | ||
| // Address is a list of seed brokers. The config name is singular for backward compatibility. | ||
| Address flagext.StringSliceCSV `yaml:"address"` |
There was a problem hiding this comment.
Broker addresses not trimmed, spaces cause connection failures
Medium Severity
flagext.StringSliceCSV.Set splits on commas without trimming whitespace from individual items. A common user input like "broker1:9092, broker2:9092" produces ["broker1:9092", " broker2:9092"] — note the leading space. The franz-go parseBrokerAddr function does not trim whitespace either, so the untrimmed address broker2:9092 causes DNS resolution failures. No validation or trimming is performed in Validate() or anywhere in the pipeline to catch this.


Summary
Add support for multiple Kafka seed brokers via comma-separated values in
ingest-storage.kafka.address.Changes
KafkaConfig.SeedBrokers()to parse and trim comma-separated broker addresseskgo.SeedBrokers(cfg.SeedBrokers()...)CreateTopicwith multiple brokersTesting
Fix #14303
Note
Medium Risk
Touches Kafka connection/bootstrap configuration and its parsing/validation, which can impact ingest availability if misconfigured. Changes are scoped and covered by updated tests and docs.
Overview
Adds support for configuring multiple Kafka seed brokers for ingest storage by changing
ingest-storage.kafka.addressfrom a single string to a CSV-backed list and wiring it through Kafka client creation (kgo.SeedBrokers(cfg.Address...)).Updates validation, tests, fixture generation, and various ingest/distributor test setups to use the new list type, and refreshes CLI help + docs + changelog to document the comma-separated format.
Written by Cursor Bugbot for commit 8009b06. This will update automatically on new commits. Configure here.