Ensure that number of partitions are correct - #6106
Closed
mattdurham wants to merge 20 commits into
Closed
Conversation
mattdurham
commented
Dec 19, 2025
| Value: &defaultNumberOfPartitions, | ||
| }, | ||
| }) | ||
| ctx := context.Background() |
Contributor
Author
There was a problem hiding this comment.
Not entirely in love with using background here but its taken from internal sources so seems legit
mattdurham
marked this pull request as ready for review
January 8, 2026 13:52
mattdurham
requested review from
carles-grafana,
electron0zero,
ie-pham,
javiermolinar,
joe-elliott,
mdisibio,
oleg-kozlyuk-grafana,
ruslan-mikhailov,
stoewer,
yvrhdn and
zalegrala
as code owners
January 8, 2026 13:52
Contributor
Author
|
The E2E tests arent happy with this change, digging into the call order for the tests. |
After creating or updating Kafka topics with many partitions (e.g., 1000), the Kafka group coordinator needs time to initialize before consumers can successfully fetch offsets. Without this check, consumers get repeated COORDINATOR_NOT_AVAILABLE errors, causing delays and test timeouts. Changes: - Add waitForCoordinator() method to verify coordinator availability - Calculate timeout dynamically based on partition count (5-60 seconds) - Use exponential backoff when checking coordinator status - Call coordinator check after all topic creation/update operations - Gracefully degrade with warnings if coordinator isn't ready in time This should resolve the E2E test timeouts in: - test-e2e-storage (TestEncodings, TestBackendScheduler) - test-e2e-limits (TestIngestionLimits) - test-e2e-operations (TestLiveStoreLookback) Generated with help from AI
mattdurham
force-pushed
the
have_kafka_partitions_be_recreated
branch
from
February 4, 2026 19:20
c591db7 to
fcfbf3b
Compare
The test was closing channels on every OffsetCommit/OffsetFetch call, but with the coordinator check potentially triggering multiple offset operations, the channels were being closed more than once causing a panic. Fix: Use sync.Once to ensure channels are only closed once, making the test resilient to multiple offset commits during the coordinator wait period. Generated with help from AI
Changed timeout calculation from partitions/10 to partitions/100 and reduced max timeout from 60s to 30s. This significantly speeds up tests with many partitions while still providing sufficient time for coordinator initialization. For 1000 partitions: 60s → 10s wait time This prevents E2E tests with multiple restarts from timing out. Generated with help from AI
Added WaitForCoordinatorMinPartitions config (default 500) to control when to wait for Kafka coordinator readiness. Tests can set this to 0 to disable coordinator waiting entirely, avoiding unnecessary delays. Changes: - Added WaitForCoordinatorMinPartitions config field (default 500) - Skip coordinator wait when topic already exists with correct partitions - Tests now set AutoCreateTopicDefaultPartitions=2 and WaitForCoordinatorMinPartitions=0 - Fixes TestBlockbuilder_PartitionWithNoLag and other timing-sensitive tests Generated with help from AI
Generated with help from AI
Contributor
|
💻 Deploy preview available (Ensure that number of partitions are correct): |
Changed WaitForCoordinatorMinPartitions to be test-only: - Not exposed as CLI flag or YAML config - Not in documentation manifest - Tests set to -1 to disable coordinator wait - Production uses default of 500 partitions This keeps the testing flexibility without exposing an internal implementation detail to users. Generated with help from AI
Consolidation: - Removed redundant waitForCoordinatorAfterUpdate function - waitForCoordinator now handles all cases with threshold check Logging reduction (only log when taking action or encountering errors): - Removed "topic already has correct partitions" log (happens every startup) - Removed "topic has more partitions than desired" log (no action taken) - Removed "skipping coordinator wait" log (happens every startup with < 500 partitions) Kept important logs: - "successfully created topic" (action taken) - "successfully updated partitions" (action taken) - "waiting for group coordinator" (action in progress) - All warning/error logs Generated with help from AI
Removed all coordinator wait logic to simplify the code and rely on the Kafka client's built-in retry mechanisms for handling transient coordinator unavailability. Changes: - Removed waitForCoordinator() function and all calls to it - Removed waitForCoordinatorAfterUpdate() function - Removed getCoordinatorWaitMinPartitions() helper - Removed WaitForCoordinatorMinPartitions field from KafkaConfig - Removed test-specific coordinator wait configuration The franz-go client already has robust retry logic that will handle transient COORDINATOR_NOT_AVAILABLE errors naturally. Generated with help from AI
Simplest possible solution: just wait 2 seconds after creating a new topic to give the Kafka coordinator time to initialize. No complex logic, no configuration, no threshold checks. This is much simpler than the previous approach and should be sufficient for coordinator initialization. Generated with help from AI
The default auto_create_topic_default_partitions is 1000, which is appropriate for production but causes E2E tests to time out because: - Creating 1000 partitions takes 30+ seconds - Coordinator initialization for 1000 partitions is slow - Tests were failing with "records have timed out before they were able to be produced" This change sets auto_create_topic_default_partitions: 2 in the E2E test base config, which: - Reduces partition creation time to < 1 second - Allows coordinator to initialize quickly - Fixes all timeout-related test failures Test results: - Before: 4/7 tests FAILED with timeouts - After: 7/7 tests PASSED in 32 seconds Production default remains 1000 partitions (unchanged). Generated with help from AI
Following the principle of only logging failures and odd states, removed info-level logs for successful operations: - "ensuring topic partitions" (normal startup) - "topic already exists" (common case) - "successfully created topic" (success) - "successfully updated partitions" (success) - "waiting for topic to be visible in metadata" (normal operation) - "topic is now visible in metadata" (success) Kept only: - "skipping topic partition setup" (unusual configuration) - "topic not yet visible, retrying" (odd state indicating slow metadata propagation) - Error returns for actual failures This reduces log noise while preserving visibility into problems. Generated with help from AI
mattdurham
commented
Feb 6, 2026
| kafka: | ||
| address: kafka:9092 | ||
| topic: tempo-ingest | ||
| auto_create_topic_default_partitions: 2 |
Contributor
Author
There was a problem hiding this comment.
Changed this since 1000 took a while to consolidate in the test harness, with these changes.
Document why sync.Once guards are necessary when closing channels in Kafka operation hooks (OffsetFetch/OffsetCommit). Multiple offset operations can occur during tests when: - Multiple consume cycles run (each cycle fetches/commits offsets) - Coordinator checks or other Kafka operations trigger additional calls Without sync.Once, attempting to close an already-closed channel causes a panic. The guards prevent this race condition and make the test robust to timing variations. Reference: commit ca6c83c "Fix race condition in TestBlockbuilder_gracefulShutdown" Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Expand the comment explaining the 2-second sleep after topic creation to clarify: - What it prevents: coordinator-not-available errors in E2E tests - When it matters: topics created with many partitions - Why it's needed: coordinator needs time to initialize before consumers can fetch offsets - Historical context: commit a03809a Verified necessity by testing: - Unit tests pass without the sleep - E2E integration tests timeout without it (600s timeout) The 2-second cost during topic creation prevents 10-minute E2E test timeouts, making this a justified performance tradeoff. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Contributor
|
This PR has been automatically marked as stale because it has not had any activity in the past 60 days. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Sometimes we see topics with one partition, which causes issues. This code is very similar to other systems used in Grafana to ensure the number of topics is correct. Whomever is review this let me know and I can send you where I pulled it from.