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: use single configuration for fetch concurrency #10156

Merged

Conversation

dimitarvdimitrov
Copy link
Contributor

What this PR does

This replaces the two previous flags with one single one.

# old flags
-ingest-storage.kafka.startup-fetch-concurrency
-ingest-storage.kafka.ongoing-fetch-concurrency

# new flag
-ingest-storage.kafka.fetch-concurrency-max

At GL we've been running with a single value for both. The reason for having two flags in the first place is because we couldn't find a balance between throughput and latency. This was largely fixed with the change to not issue fetch requests for beyond the HWM (#9892). So now we no longer need the complexity of being able to change concurrency settings

Which issue(s) this PR fixes or relates to

Fixes #

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.

@dimitarvdimitrov dimitarvdimitrov requested review from tacole02 and a team as code owners December 6, 2024 15:50
@@ -71,7 +70,6 @@
* [ENHANCEMENT] Compactor: refresh deletion marks when updating the bucket index concurrently. This speeds up updating the bucket index by up to 16 times when there is a lot of blocks churn (thousands of blocks churning every cleanup cycle). #9881
* [ENHANCEMENT] PromQL: make `sort_by_label` stable. #9879
* [ENHANCEMENT] Distributor: Initialize ha_tracker cache before ha_tracker and distributor reach running state and begin serving writes. #9826 #9976
* [ENHANCEMENT] Ingester: `-ingest-storage.kafka.max-buffered-bytes` to limit the memory for buffered records when using concurrent fetching. #9892
Copy link
Contributor Author

@dimitarvdimitrov dimitarvdimitrov Dec 6, 2024

Choose a reason for hiding this comment

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

i've removed this item because they're not changes compared to the last released version because concurrent fetching and ingestion haven't been released yet

@@ -16,7 +16,6 @@
* [CHANGE] Distributor: Drop experimental `-distributor.direct-otlp-translation-enabled` flag, since direct OTLP translation is well tested at this point. #9647
* [CHANGE] Ingester: Change `-initial-delay` for circuit breakers to begin when the first request is received, rather than at breaker activation. #9842
* [CHANGE] Query-frontend: apply query pruning before query sharding instead of after. #9913
* [CHANGE] Ingester: remove experimental flags `-ingest-storage.kafka.ongoing-records-per-fetch` and `-ingest-storage.kafka.startup-records-per-fetch`. They are removed in favour of `-ingest-storage.kafka.max-buffered-bytes`. #9906
Copy link
Contributor Author

@dimitarvdimitrov dimitarvdimitrov Dec 6, 2024

Choose a reason for hiding this comment

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

i've removed this item because they're not changes compared to the last released version because concurrent fetching and ingestion haven't been released yet

This replaces the two previous flags with one single one.

```
# old flags
-ingest-storage.kafka.startup-fetch-concurrency
-ingest-storage.kafka.ongoing-fetch-concurrency

# new flag
-ingest-storage.kafka.fetch-concurrency-max
```

At GL we've been running with a single value for both. The reason for having two flags in the first place is because we couldn't find a balance between throughput and latency. This was largely fixed with the change to not issue fetch requests for beyond the HWM (9892). So now we no longer need the complexity of being able to change concurrency settings

Signed-off-by: Dimitar Dimitrov <dimitar.dimitrov@grafana.com>
@dimitarvdimitrov dimitarvdimitrov force-pushed the dimitar/ingest/replay-speed/remove-multiple-options branch from 60e42ef to fb1082f Compare December 6, 2024 15:52
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.

Very happy to see this complexity going away! ;)

@chencs
Copy link
Contributor

chencs commented Dec 10, 2024

The CHANGELOG has just been cut to prepare for the next release. Please rebase main and eventually move the CHANGELOG entry added / updated in this PR to the top of the CHANGELOG.md document. Thanks!

@dimitarvdimitrov dimitarvdimitrov merged commit d39271a into main Dec 11, 2024
30 checks passed
@dimitarvdimitrov dimitarvdimitrov deleted the dimitar/ingest/replay-speed/remove-multiple-options branch December 11, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants