asdf2014 commented on PR #22963: URL: https://github.com/apache/kafka/pull/22963#issuecomment-5206782318
Agreed, and thanks for pushing back on this. I have reworked the patch to bound the read inside `FileRecords#readInto` and reverted the two call site changes. It now takes its bound from `availableBytes`, the same helper `slice` and `sliceUnaligned` use, so the three of them agree on what a position past the end means instead of each having its own policy. The javadoc states that a buffer with more room than the records left from the given position has its limit lowered, and that the position is rejected when it is negative or past the end. The old `@param position` line described a position in the buffer, which is not what the code does, so that is corrected too. `FileRecordsTest.testReadIntoStopsAtTheEndOfTheRecords` is new and pins the contract where it now lives: a slice is read up to its end and not into the records that follow it, a slice that does not start at the beginning of the file reads from its own start, a preallocated file is read up to its records and not into the unwritten tail, reading at the end yields nothing, and an out of range position is rejected. For the other callers: `LogSegment#appendChunkFromFile` sets the limit itself before reading and `AbstractFetcherThread#toMemoryRecords` allocates exactly `sizeInBytes`, so neither changes. The log cleaner passes an oversized buffer but only ever reads non active segments, which are physically trimmed when they roll, so its reads already stopped at the same byte. `RecordsIterator` does change: it reads a larger slice in chunks, and the last chunk was bounded only by the room left in its buffer rather than by what was left of the slice. I could not construct a failing case for it, and the metadata log is never preallocated so there is no unwritten tail to run into, but the comment above `firstBatchSize()` was resting on the old contract, so I corrected it. -- 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]
