Repository navigation
Conversation
…rom as such Follow-up to apache#23445, under the same ticket (resolved, fix version 4.5.0). Since apache#23445, a failed send of fetched records is passed to a handler that re-reads the region on the scheduler, and the log directory is offlined only if the re-read fails too. The re-read is needed because over PLAINTEXT `sendfile` reads the file and writes the socket in one call, so a disk error and a socket error are the same bare `IOException`. `SslTransportLayer.transferFrom` does not have that problem: it reads the file with `FileChannel.read`, then encrypts and writes, so it knows which of the two failed. As discussed on apache#23445, this passes that on instead of finding it out again: * `SslTransportLayer.transferFrom` throws a new `FileReadException`, an `IOException` with the position of the read, when `FileChannel.read` or `FileChannel.size` fails. A `ClosedChannelException`, from a deletion or a shutdown, is rethrown as is. * `TransferableChannel.reportsFileReadFailures()` says whether a transport does that: true for TLS, false, the default, for PLAINTEXT. * `FileRecords.writeTo` passes a `FileReadException` to the handler, and any other failure only if the transport cannot tell the two apart. A failure of its own `channel.size()` call before the transfer becomes a `FileReadException` as well. * `LocalLog` reports a `FileReadException` as a failure of the log directory without re-reading the region, on the scheduler and at most one report at a time per log directory, as for the re-read. Reports are tracked apart from re-reads, so that a re-read running in the same directory does not drop a known failure. Other failures are re-read as before. Consequences worth stating: * On TLS listeners a disconnect no longer re-reads the region that was being sent. In the test notes, consumers killed mid-fetch over TLS for 5 minutes made the scheduler re-read 151 MB without this change and nothing with it. * On TLS listeners a read error now offlines the log directory even if the region could be read again by the time of a re-read, e.g. after a transient media error. Before, such an error was attributed to the socket. This is the criterion `LocalLog` applies to any other `IOException`. * A failed `FileChannel.size()` call on the segment, over either transport, is now reported as a read failure without a re-read. * PLAINTEXT listeners still re-read: `sendfile` cannot tell the two apart. * KRaft snapshots, sent through `UnalignedFileRecords`, are still not covered. Generated-by: Claude (Anthropic)
|
Test notes for 73fc46a, on trunk d18f93f. Unit tests:
Cluster: 3 brokers and a controller (KRaft) in Docker on a Linux 6.8
With this change the failed send is still logged as a disconnect, at |
|
Test notes, continued: the same on macOS 26.7 (Apple Silicon, JDK 17),
On macOS a read fails before the unreadable bytes: with the band at |
|
Test notes, continued: OpenBSD 8.0 (arm64, QEMU on macOS) and Windows
No InternalError was thrown in any run. |
|
Windows is on a best-effort basis today. Too different memory management/disk read error handling by the NT kernel and surrounding libraries. I will probably take a deeper look after this PR. Maybe, we can bring some better support for Windows. |
Follow-up to #23445, under the same ticket (resolved, fix version
4.5.0).
Since #23445, a failed send of fetched records is passed to a handler
that re-reads the region on the scheduler, and the log directory is
offlined only if the re-read fails too. The re-read is needed because
over PLAINTEXT
sendfilereads the file and writes the socket in onecall, so a disk error and a socket error are the same bare
IOException.SslTransportLayer.transferFromdoes not have thatproblem: it reads the file with
FileChannel.read, then encrypts andwrites, so it knows which of the two failed. As discussed on #23445,
this passes that on instead of finding it out again:
SslTransportLayer.transferFromthrows a newFileReadException, anIOExceptionwith the position of the read, whenFileChannel.reador
FileChannel.sizefails. AClosedChannelException, from adeletion or a shutdown, is rethrown as is.
TransferableChannel.reportsFileReadFailures()says whether atransport does that: true for TLS, false, the default, for
PLAINTEXT.
FileRecords.writeTopasses aFileReadExceptionto the handler,and any other failure only if the transport cannot tell the two
apart. A failure of its own
channel.size()call before the transferbecomes a
FileReadExceptionas well.LocalLogreports aFileReadExceptionas a failure of the logdirectory without re-reading the region, on the scheduler and at most
one report at a time per log directory, as for the re-read. Reports
are tracked apart from re-reads, so that a re-read running in the
same directory does not drop a known failure. Other failures are
re-read as before.
Consequences worth stating:
being sent. In the test notes, consumers killed mid-fetch over TLS
for 5 minutes made the scheduler re-read 151 MB without this change
and nothing with it.
the region could be read again by the time of a re-read, e.g. after a
transient media error. Before, such an error was attributed to the
socket. This is the criterion
LocalLogapplies to any otherIOException.FileChannel.size()call on the segment, over eithertransport, is now reported as a read failure without a re-read.
sendfilecannot tell the twoapart.
UnalignedFileRecords, are still notcovered.
Generated-by: Claude (Anthropic)