junrao commented on code in PR #23928:
URL: https://github.com/apache/kafka/pull/23928#discussion_r4380281579
##########
clients/src/main/java/org/apache/kafka/common/record/internal/FileRecords.java:
##########
@@ -63,7 +67,7 @@ public interface TransferFailureHandler {
private final boolean isSlice;
private final int start;
private final int end;
- // Notified when writing to a network channel fails, or null
+ // Notified when writing to a network channel fails as described in
TransferFailureHandler, or null
Review Comment:
or null => if not null?
##########
clients/src/main/java/org/apache/kafka/common/record/internal/FileRecords.java:
##########
@@ -325,30 +330,54 @@ public int writeTo(TransferableChannel destChannel, int
offset, int length) thro
long position = start + offset;
int count = Math.min(length, oldSize - offset);
try {
- long newSize = Math.min(channel.size(), end) - start;
+ long newSize = Math.min(fileSize(position), end) - start;
if (newSize < oldSize)
throw new KafkaException(String.format(
"Size of FileRecords %s has been truncated during
write: old size %d, new size %d",
file.getAbsolutePath(), oldSize, newSize));
// safe to cast to int since `count` is an int
return (int) destChannel.transferFrom(channel, position, count);
} catch (IOException e) {
- // The transfer reads the file and writes the socket in one call,
so a storage failure and a socket
- // failure both arrive here as a bare IOException. Let the handler
find out which one it was. A closed
- // channel means the file was not read, so there is nothing to
find out.
- if (transferFailureHandler != null && !(e instanceof
ClosedChannelException))
+ if (transferFailureHandler != null &&
mayBeReadFailure(destChannel, e))
Review Comment:
mayBeReadFailure => mayBeFileReadFailure ?
##########
clients/src/main/java/org/apache/kafka/common/network/SslTransportLayer.java:
##########
@@ -1003,8 +1003,9 @@ private void updateBytesBuffered(boolean madeProgress) {
* Transfers bytes from `fileChannel` by reading them into a buffer and
writing them to this channel, since
* they have to be encrypted.
*
- * @throws IOException If the file cannot be read or this channel cannot
be written. The bytes the failed call
- * already wrote are not reported, so this channel must not be
used after it.
+ * @throws FileReadException If the file cannot be read, other than
because it was closed
+ * @throws IOException If this channel cannot be written to, or the file
was closed. Whatever the failed call
Review Comment:
The sentence starting with "Whatever" applies to all exceptional cases, not
just IOException. Also, if the file was closed, should we say explicitly that a
ClosedChannelException is thrown?
##########
clients/src/main/java/org/apache/kafka/common/network/FileReadException.java:
##########
@@ -0,0 +1,64 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.common.network;
+
+import java.io.IOException;
+import java.nio.channels.ClosedChannelException;
+
+/**
+ * Thrown when the file being transferred to a network channel could not be
read, its size included, as opposed to
+ * the network channel not being written to: by {@link
TransferableChannel#transferFrom} if the channel
+ * {@link TransferableChannel#distinguishesFileReadFailures()}, and by the
records being transferred. A file channel
Review Comment:
I don't quite understand why "by the records being transferred" is here.
--
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]
