-
Notifications
You must be signed in to change notification settings - Fork 475
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-10821. Ensure ozone write all the buffer content to FileChannel. #6652
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @duongkame for the patch.
* Write all the contents of the buffer from the current position to the limit | ||
* to {@code channel}. | ||
*/ | ||
void writeFully(GatheringByteChannel channel) throws IOException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to suggest fixing writeTo
instead of adding this new method to the interface.
-
In what case may we want to use the existing method? With this fix,
writeTo
becomes unused in production. -
TestChunkBuffer.assertWrite
testswriteTo
and expects it to write all data. However, it only works because the channels it uses, unlikeFileChannel
, write fully. This can be verified by randomly adjustinglimit
for the buffer inMockGatheringChannel.write(ByteBuffer)
, i.e. simulating incomplete writes. On the other hand,writeFully
is not covered by unit tests. -
We can avoid the changes in
ChunkUtils
, most of which is due towriteFully
not returning the number of bytes written.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@duongkame you can pick adoroszlai@daee756, which implements the suggested change
throws IOException | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/utils/BufferUtils.java
149: '{' at column 3 should be on the previous line.
if (n <= 0) | ||
throw new IllegalStateException("no bytes written"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/utils/BufferUtils.java
152: 'if' construct must use '{}'s.
What changes were proposed in this pull request?
Ozone should expect
FileChannel.write
doesn't fully write the entire buffer and not crash the pipeline one that happens.See HDDS-10821.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10821
How was this patch tested?
CI.