kamalcph commented on PR #21108: URL: https://github.com/apache/kafka/pull/21108#issuecomment-3659560571
Thanks for the PR! Went over first pass, have below questions: > Startup race condition: RemoteLogManager initialized with default quotas (Long.MAX_VALUE = unlimited) and relied on dynamic config updates to apply correct values, creating a window where operations could exceed configured quotas. Could you add a unit test to cover this case? > Multiple threads could check quota before any recorded usage, allowing all to bypass limits simultaneously. Also, cover this case with a unit test to understand the issue. > Modified ReplicaManager to call recordAndCheckFetchQuota() BEFORE dispatching remote fetch, If the remoteFetchQuotaBytesPerSecond is set to 25 Mbps and there is a message with 30 MB size, will the consumer get stuck? The previous behaviour was to allow the consumption to continue. > On error, releases the full reservation to prevent leaks How can Kafka differentiate whether the error is from remote storage or there is an error in processing the response? I think this is a good improvement. Usually, we don't expect errors in RLMM or any Kafka components while processing the response. -- 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]
