SEPURI-SAI-KRISHNA commented on PR #23211: URL: https://github.com/apache/kafka/pull/23211#issuecomment-5353914394
Heads-up on the interaction with #23206 (KAFKA-20966), which touches the same method. The two changes are independent in substance — that one adds a `maxMessageSize` bound to `RemoteLogInputStream`, this one fixes the stream close on the exception path — but they overlap in `RemoteLogManager.read()` in two spots: - at the `getRemoteLogInputStream(...)` call, where #23206 changes the call and this PR adds `remoteSegInputStream = null;` three lines below it - at the `finally` block this PR rewrites, immediately above the `getRemoteLogInputStream` helper whose signature #23206 changes Both resolve by simply taking each side's change. One thing a merge will *not* flag: #23206 also updates the test override to `getRemoteLogInputStream(InputStream in, int maxMessageSize)`, while the test added here (`testReadClosesRemoteSegmentInputStreamWhenReadingBatchFails`) declares its own `@Override` of the one-arg form. Those two points are far enough apart in the file that the merge should succeed cleanly but then fail to compile, so whichever PR lands second needs that override updated to the two-arg signature. Happy to rebase this one on top of #23206, or the other way round, if that makes review easier — just let me know which you'd prefer to take first. -- 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]
