You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2021/01/26 02:23:53 UTC
[GitHub] [kafka] dengziming opened a new pull request #9970: Kafka 12233: `FileRecords.writeTo` set length incorrectly
dengziming opened a new pull request #9970:
URL: https://github.com/apache/kafka/pull/9970
*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)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #9970: Kafka 12233: `FileRecords.writeTo` set length incorrectly
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#issuecomment-767242636
Hi, @junrao , PTAL.
This is changed in #2140 , also ping @chia7712 @hachikuji to check 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on a change in pull request #9970: Kafka-12233: `FileRecords.writeTo` set length incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 commented on a change in pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#discussion_r564762299
##########
File path: clients/src/main/java/org/apache/kafka/common/record/FileRecords.java
##########
@@ -278,7 +278,7 @@ public long writeTo(TransferableChannel destChannel, long offset, int length) th
file.getAbsolutePath(), oldSize, newSize));
long position = start + offset;
- int count = Math.min(length, oldSize);
+ long count = Math.min(length, oldSize - offset);
Review comment:
> 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).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #9970: Kafka 12233: `FileRecords.writeTo` set length incorrectly
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#issuecomment-767242636
Hi, @junrao , PTAL.
This is changed in #2140 , also ping @chia7712 @hachikuji to check 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] junrao commented on a change in pull request #9970: Kafka-12233: `FileRecords.writeTo` set length incorrectly
Posted by GitBox <gi...@apache.org>.
junrao commented on a change in pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#discussion_r564747239
##########
File path: clients/src/main/java/org/apache/kafka/common/record/FileRecords.java
##########
@@ -278,7 +278,7 @@ public long writeTo(TransferableChannel destChannel, long offset, int length) th
file.getAbsolutePath(), oldSize, newSize));
long position = start + offset;
- int count = Math.min(length, oldSize);
+ long count = Math.min(length, oldSize - offset);
Review comment:
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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9970: Kafka-12233: `FileRecords.writeTo` set length incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#issuecomment-768023584
@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"?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #9970: Kafka-12233: `FileRecords.writeTo` set length incorrectly
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#issuecomment-767592824
@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.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dengziming commented on pull request #9970: Kafka-12233: Align the length passed to FileChannel by `FileRecords.writeTo`
Posted by GitBox <gi...@apache.org>.
dengziming commented on pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#issuecomment-768026154
@chia7712 Thank you for your suggestions, changed the title. Maybe we can discuss the `IllegalStateException` and address them in follow-up patches.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 merged pull request #9970: KAFKA-12233: Align the length passed to FileChannel by `FileRecords.writeTo`
Posted by GitBox <gi...@apache.org>.
chia7712 merged pull request #9970:
URL: https://github.com/apache/kafka/pull/9970
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] chia7712 commented on pull request #9970: Kafka-12233: `FileRecords.writeTo` set length incorrectly
Posted by GitBox <gi...@apache.org>.
chia7712 commented on pull request #9970:
URL: https://github.com/apache/kafka/pull/9970#issuecomment-767313055
@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?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org