KAFKA-12233: Align the length passed to FileChannel by FileRecords.writeTo

[dengziming]

More detailed description of your change
See https://issues.apache.org/jira/browse/KAFKA-12233

Summary of testing strategy (including rationale)
unit test

Committer Checklist (excluded from commit message)

• Verify design and implementation
• Verify test coverage and CI build status
• Verify documentation (including upgrade notes)

[dengziming]

Hi, @junrao , PTAL.
This is changed in #2140 , also ping @chia7712 @hachikuji to check it.

[chia7712]

@dengziming Thanks for your patch.

Does this patch want to adjust "count" to make sure it is NOT out of bound of file size? If so, it may be redundant as the "count" is maximum number to transfer rather than the "absolute" number to transfer. It seems to me all implementation of TransferableChannel should adjust the "count" like FileChannel#transferTo. In other words, the transferred data should be fewer than the "count" if the channel contains fewer than "count". Also, That behavior is consistent to read(ByteBuffer[] dsts, int offset, int length). WDYT?

[dengziming]

@chia7712 You are right, in fact all implementation of TransferableChannel will ensure remaining= sizeInBytes() - previouslyWritten, which indicates length = oldSize - offset, so this pr is redundant. but I think Math.min(length, oldSize - offset) is better than Math.min(length, oldSize) since the latter will also not work.

[junrao]

@dengziming : Thanks for the PR. Left a comment below.

[junrao]

This code is called from SocketServer through RecordsSend. The expectation is that the underlying file channel has enough bytes to return for the requested length. Normally this should be the case. If for some reason this is not true, by bounding the transfer amount by the remaining bytes in the file channel will cause RecordsSend.writeTo() to be called in a loop until the request times out. This may hide a real problem.

Perhaps what we could do is to throw an IllegalStateException if the file channel doesn't have enough bytes left. This will cause the socket channel to be closed and the Selector to log a warning.

[chia7712]

Perhaps what we could do is to throw an IllegalStateException if the file channel doesn't have enough bytes left. This will cause the socket channel to be closed and the Selector to log a warning.

Please update docs for related writeTo methods if we want to apply this behavior. The current code use return value to calculate the length for next write. The return value gets redundant since the number of transferred data is always equal to expected length (it throws exception when there is no enough bytes left).

[chia7712]

@dengziming Thanks for nice #9819 and aligning the count is good to me.

[chia7712]

@dengziming Could you revise the title? It seems to me the passed length is NOT incorrect but it has room to tweak the value. How about "align the length passed to FileChannel by FileRecords.writeTo"?

[dengziming]

@chia7712 Thank you for your suggestions, changed the title. Maybe we can discuss the IllegalStateException and address them in follow-up patches.