refactor(inkless): clarify fetch routing for consolidating partitions#633
Merged
Conversation
jeqo
marked this pull request as ready for review
June 5, 2026 13:01
jeqo
force-pushed
the
jeqo/refactor-fetch-messages
branch
2 times, most recently
from
June 5, 2026 13:17
50b6832 to
b8379f5
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors ReplicaManager.fetchMessages routing logic for diskless vs classic fetches (including consolidating partitions), aiming to improve readability and add more diagnostic logging when partition lookup / fetch flows fail.
Changes:
- Simplifies diskless fetch routing by removing a separate “invalid consolidating” response buffer and folding those into immediate responses.
- Moves topic-id backfill to the routing decision point (and introduces a helper for backward-compatible topic-id backfill).
- Adds additional warnings when partition lookup fails or when diskless fetch is rejected due to managed replicas being disabled.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeqo
force-pushed
the
jeqo/refactor-fetch-messages
branch
3 times, most recently
from
June 5, 2026 13:51
9bbc0b0 to
92aaf6e
Compare
Comment on lines
+2111
to
+2116
| if (classicFetchInfos.isEmpty && disklessFetchInfos.isEmpty && immediateFetchResponses.nonEmpty) { | ||
| respond(Seq.empty) | ||
| return | ||
| } | ||
|
|
||
| if (classicFetchInfos.isEmpty && disklessFetchInfosWithoutTopicId.isEmpty && | ||
| immediateFetchResponses.isEmpty && invalidConsolidatingPartitionFetchResponses.nonEmpty) { | ||
| if (classicFetchInfos.isEmpty && disklessFetchInfos.isEmpty && immediateFetchResponses.isEmpty) { |
Contributor
There was a problem hiding this comment.
I think you can collapse these into one if (classicFetchInfos.isEmpty && disklessFetchInfos.isEmpty) because we respond(Seq.empty) regardless of the value of immediateFetchResponses.
Contributor
Author
There was a problem hiding this comment.
Good catch, even better.
jeqo
force-pushed
the
jeqo/refactor-fetch-messages
branch
from
June 5, 2026 14:49
92aaf6e to
d9aac76
Compare
viktorsomogyi
approved these changes
Jun 5, 2026
jeqo
added a commit
that referenced
this pull request
Jun 5, 2026
jeqo
added a commit
that referenced
this pull request
Jun 5, 2026
jeqo
added a commit
that referenced
this pull request
Jun 5, 2026
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
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.
Some minor changes to make fetch messages more readable: