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

ingest: manually create Kafka topic instead of configuring num.partitions #10101

Merged

Conversation

dimitarvdimitrov
Copy link
Contributor

@dimitarvdimitrov dimitarvdimitrov commented Dec 3, 2024

What this PR does

This PR does two things:

1. Manually creates the Kafka topic instead of relying on kgo autocreation

Here’s why kgo autocreation doesn’t work for us.

  • autocreation can only happen on metadata requests (for example kgo.Client.ForceMetadataRefresh() and kadm.Client.Metadata() ). There is a field on the request called AllowAutoTopicCreation
  • kgo would set AllowAutoTopicCreation in the request only when doing kgo.Client.ForceMetadataRefresh, but not when doing kadm.Client.Metadata
  • the field makes sense only if the metadata request contains some list of topics
  • the metadata request can only contain a list of topics if the kgo.Client has seen them before - either by consuming from them or producing to them

In other words, for the topic to be created we need the distributor to push records or the ingester to consume records. With concurrent fetching we can't consume until we have a topic. The distributor waits for the ingester to become ready (and register itself in the rings) before producing. This created a cycling dependency between the ingester and distributor.

Because of this we now issue an explicit CreateTopics request.

As a result we no longer configure num.partitions in the Kafka cluster

2. Enables ingestion and fetching concurrency by default in e2e tests

This is one of the last steps before making these the defaults in Mimir.

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

This PR does three things:
1. handles unknown topic/partition errors when seeking to the right offset when starting concurrentFetchers.
2. handles unknown topic/partition errors when getting the topic ID when starting concurrentFetchers
3. enables concurrent fetching and ingestion in e2e tests

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov requested a review from a team as a code owner December 3, 2024 16:48
@pracucci pracucci self-requested a review December 4, 2024 08:28
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov changed the title ingest: don't fail on missing topics, run in e2e tests ingest: manually create Kafka topic Dec 4, 2024
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov changed the title ingest: manually create Kafka topic ingest: manually create Kafka topic instead of configuring num.partitions Dec 4, 2024
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Copy link
Collaborator

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your patience 🙏

Makefile Outdated Show resolved Hide resolved
pkg/storage/ingest/util.go Show resolved Hide resolved
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov merged commit 36da907 into main Dec 6, 2024
29 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/ingest/replay-speed/enable-in-end-to-end-tests branch December 6, 2024 09:10
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