-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
fix(distributor): validate partition ring is kafka is enabled #14303
fix(distributor): validate partition ring is kafka is enabled #14303
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving but see the comment.
pkg/loki/modules.go
Outdated
@@ -1753,7 +1753,7 @@ func (t *Loki) initAnalytics() (services.Service, error) { | |||
|
|||
// The Ingest Partition Ring is responsible for watching the available ingesters and assigning partitions to incoming requests. | |||
func (t *Loki) initPartitionRing() (services.Service, error) { | |||
if !t.Cfg.Ingester.KafkaIngestion.Enabled { | |||
if !t.Cfg.Ingester.KafkaIngestion.Enabled && !t.Cfg.Distributor.KafkaEnabled { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes sense but the distributor won't be able to send any data to Kafka unless the ingester is also enabled due to needing some active partitions. I wonder if we should just use a single flag to enable the kafka write path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand although it feels weird that having t.Cfg.Distributor.KafkaEnabled
without t.Cfg.Ingester.KafkaIngestion.Enabled
panics ? Should we validate instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, I think we need to validate that ingestion is enabled if distributor is enabled, since they're coupled in the architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To clarify, I think we want to keep both flags because we want the option to stop writing to Kafka without changing the ingesters.
But we also want a validation for All
(and maybe Write
) mode to enforce that the ingester is enabled if the distributor is enabled. We don't want this check for micro-services mode though, since those flags can be set explicitly on those components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok I'll add a validation. That will avoid the panic at least.
What this PR does / why we need it:
I think it makes more sense to also enable partition ring when distributor push kafka is enabled
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Checklist
CONTRIBUTING.md
guide (required)feat
PRs are unlikely to be accepted unless a case can be made for the feature actually being a bug fix to existing behavior.docs/sources/setup/upgrade/_index.md
production/helm/loki/Chart.yaml
and updateproduction/helm/loki/CHANGELOG.md
andproduction/helm/loki/README.md
. Example PRdeprecated-config.yaml
anddeleted-config.yaml
files respectively in thetools/deprecated-config-checker
directory. Example PR