apoorvmittal10 commented on PR #23892: URL: https://github.com/apache/kafka/pull/23892#issuecomment-6325969960
> > Thanks for the PR. An AI-assisted review suggests this PR is a regression in behaviour. > > The skip drops the partitions that currently have first claim on the budget (DelayedShareFetch.java:849). The new comment says "the already fetched records are handed out first". They are not. The merge at the end of `combineLogReadResponse` does `missingTopicPartitionsLogReadResponse.putAll(existingFetchedData)`, so the missing partitions sit first in the combined map, and processFetchResponse acquires in map order with `maxFetchRecords - acquiredRecordsCount`. So before this change the missing partitions were acquired first and the already-fetched ones got the remainder. After it, the already-fetched span is used to decide the missing ones get nothing. Concrete scenario, one consumer, tp0 and tp1 both with deep backlogs: > > > > * tp0 is served, so its next fetch offset moves and its cached fetch-offset metadata goes stale. It is read in tryComplete and lands in the already-fetched set every time. > > * tp1 is not served, so its offset and cached metadata stay valid. It is never read in tryComplete and is always a "missing" partition. > > * Before the change, tp1 was acquired first on each fetch and the two alternated naturally. After it, tp0's span is at least maxFetchRecords, so tp1 is skipped on every fetch for as long as tp0 has a backlog. That is indefinite starvation of tp1. > > Yes it's correct that unfortunately that used to happen. But it was not intended, correct me if I am wrong @adixitconfluent. We make best effort basis to acquire and process the partitions in order of partitions in the request. This additional case shall trigger when some partitions are fetched but yet some additional are needed later. However all topics are already acquired. The order is anyways not maintained in this latter case so this change should ideally not cause regression. > > Prior calling the delayedsharefetch, partitions are always rotated in share partition manager to prevent starvation. I (claude) wrote a test and can see starvation of 1 partition. It was just a matter of sequence of operations that this piece was earlier working, @adixitconfluent correct me if I am wrong, as I do not think we deliberately put the missing partitions prior to already fetched in the response. The problem is a bit deep while updating fetchOffsetMetadata for the partitions. When all fetched records are not sent to client then fetchOffsetMetadata is also bumped for trimmed partitions which inherently adds the partitions to the missing list. @AndrewJSchofield @adixitconfluent Let me know the choice, I can address that in the `same PR` or `separate PR` or `prior to current PR` (my preference). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
