adoroszlai commented on code in PR #6652:
URL: https://github.com/apache/ozone/pull/6652#discussion_r1593548741
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/ChunkBuffer.java:
##########
@@ -143,6 +143,12 @@ default ChunkBuffer put(ByteString b) {
*/
long writeTo(GatheringByteChannel channel) throws IOException;
+ /**
+ * Write all the contents of the buffer from the current position to the
limit
+ * to {@code channel}.
+ */
+ void writeFully(GatheringByteChannel channel) throws IOException;
Review Comment:
I would like to suggest fixing `writeTo` instead of adding this new method
to the interface.
1. In what case may we want to use the existing method? With this fix,
`writeTo` becomes unused in production.
2. `TestChunkBuffer.assertWrite` tests `writeTo` and expects it to write all
data. However, it only works because the channels it uses, unlike
`FileChannel`, write fully. This can be verified by randomly adjusting `limit`
for the buffer in `MockGatheringChannel.write(ByteBuffer)`, i.e. simulating
incomplete writes. On the other hand, `writeFully` is not covered by unit
tests.
3. We can avoid the changes in `ChunkUtils`, most of which is due to
`writeFully` not returning the number of bytes written.
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/utils/BufferUtils.java:
##########
@@ -136,4 +139,18 @@ public static int getNumberOfBins(long numElements, int
maxElementsPerBin) {
}
return Math.toIntExact(n);
}
+
+ /**
+ * Write all remaining bytes in buffer to the given channel.
+ * If the channel is selectable then it must be configured blocking.
+ */
+ public static void writeFully(GatheringByteChannel ch, ByteBuffer bb)
+ throws IOException
+ {
+ while (bb.remaining() > 0) {
+ int n = ch.write(bb);
+ if (n <= 0)
+ throw new IllegalStateException("no bytes written");
Review Comment:
```
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/utils/BufferUtils.java
152: 'if' construct must use '{}'s.
```
##########
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/utils/BufferUtils.java:
##########
@@ -136,4 +139,18 @@ public static int getNumberOfBins(long numElements, int
maxElementsPerBin) {
}
return Math.toIntExact(n);
}
+
+ /**
+ * Write all remaining bytes in buffer to the given channel.
+ * If the channel is selectable then it must be configured blocking.
+ */
+ public static void writeFully(GatheringByteChannel ch, ByteBuffer bb)
+ throws IOException
+ {
Review Comment:
```
hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/common/utils/BufferUtils.java
149: '{' at column 3 should be on the previous line.
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
