apoorvmittal10 commented on PR #23892: URL: https://github.com/apache/kafka/pull/23892#issuecomment-6322560062
> 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. -- 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]
