cheeeee opened a new pull request, #23445:
URL: https://github.com/apache/kafka/pull/23445
The Fetch response is sent zero copy, so the read from the log happens on the
network thread while the response is written to the socket:
```
Selector.pollSelectionKeys -> KafkaChannel.write -> NetworkSend.writeTo
-> MultiRecordsSend -> DefaultRecordsSend -> FileRecords.writeTo
-> {Plaintext,Ssl}TransportLayer.transferFrom -> FileChannel.transferTo
```
A failure of the medium and a client that went away both arrive at that call
as
a bare `java.io.IOException`, and `Selector` classifies every `IOException`
there as a disconnect, at DEBUG. A partially unreadable log file therefore
makes
the partition unreadable while the cluster believes the broker is healthy.
This change does the disambiguation the issue asks for. It does **not**
offline
the log directory yet; see "Scope" below.
### Reproduction
Released 4.3.1 distribution, single-node KRaft broker, two log directories,
one
of them on a `dm-dust` device. The tail of one segment is marked unreadable
while its head and the indexes stay in the page cache, so offset translation
succeeds and the failure can only land in the transfer. Then one large fetch
spans into the marked tail.
From the guest's tracefs:
```
data-plane-kafk-1296 sys_sendfile64(out_fd: 0xb3, in_fd: 0xaf, offset:
0xc8ffdea8, count: 0xd737a)
data-plane-kafk-1296 sys_sendfile64 -> 0xfffffffffffffffb <- -EIO
```
This needs `CONFIG_DM_DUST`, which stock distribution kernels do not enable,
so
the broker runs inside a User-Mode Linux guest built with the device-mapper
fault-injection targets.
Worth stating because it explains why the application is the only place this
can
be handled: a failed read of *file data* produces no ext4 error and no
kernel log
line, and leaves the filesystem writable — measured: `EXT4-fs error` count 0,
empty `dmesg`, and a write to the same filesystem immediately afterwards
succeeded. The errno handed to Kafka is the only signal in the stack.
### Change
On the error path of `FileRecords.writeTo`, re-read the region that failed to
transfer:
* the file still reads -> the socket was at fault; the original exception is
rethrown untouched;
* the file does not read -> `KafkaStorageException` naming the file and the
position, with the original exception as its cause.
The message text of the exception is deliberately not inspected: it is the
platform's `strerror` output, and the error sets of the underlying transfer
differ between platforms. Where the re-read cannot conclude anything — a
position at or past the end of the file — the original exception is
preserved,
which is today's behaviour. The check sits in `FileRecords`, above both
transport layers, so it covers the plaintext and the SSL paths named in the
issue; the file name and position needed for the re-read are only known
there.
Cost: one extra read, on the error path only.
### Testing
`FileRecordsTest`, three cases, driven through the test-visible constructor
with
a mocked `FileChannel` so they are deterministic on any platform:
* `testWriteToPreservesExceptionWhenFileIsStillReadable`
* `testWriteToReportsStorageFailureWhenRegionIsUnreadable`
* `testWriteToPreservesExceptionAtEndOfFile`
`FileRecordsTest` 56 tests, 0 failures; `:clients:checkstyleMain` and
`:clients:checkstyleTest` pass.
End to end, the same released 4.3.1 distribution was run twice in the guest
with
the identical scenario, differing only in this one class:
| | before | after |
|---|---|---|
| `sendfile64` returning `-EIO` | 162 | 168 |
| `KafkaStorageException` in the broker log | 0 | 168 |
| `DEBUG ... disconnected` | 170 | 8 |
| `WARN ... Unexpected error` | 0 | 168 |
| log lines naming the unreadable file | 3 | 171 |
| log directory offlined | 0 | 0 |
| broker alive at the end | yes | yes |
162 of the 170 disconnects were disk failures wearing a network label; the 8
that remain are real client disconnects. The `-EIO` counts differ slightly
between runs because which fetch lands on a marked block depends on
scheduling.
What the operator gets now:
```
WARN [SocketServer listenerType=BROKER, nodeId=1] Unexpected error from
/127.0.0.1 ...
org.apache.kafka.common.errors.KafkaStorageException: Failed to read 922390
bytes
at position 638976 of /tmp/bad/logs/t1-0/00000000000000000000.log while
writing a response
Caused by: java.io.IOException: Input/output error
```
### Scope, and what is deliberately left out
The issue asks for two things: disambiguate, and offline the log directory.
This
PR does the first. The second needs a decision I would rather take with a
committer than guess at:
1. **Where to cross the module boundary.** `clients` cannot depend on
`storage`,
so `Selector` cannot call `LogDirFailureChannel`. The cheapest route is
to let
`ChannelState` carry the cause — it already carries
`AuthenticationException`
for exactly this reason — and have
`SocketServer.Processor.processDisconnected`
route it. A listener injected into `Selector` from the server side is
cleaner
layering and more code. Which do you prefer?
2. **Granularity.** Kafka's failure unit is the log directory, so one
unreadable
4 KiB block would offline a directory of otherwise healthy replicas, to be
re-fetched over the network. Defensible, but it is a policy choice worth
making explicitly.
3. **Whether either of the above needs a KIP.** `FileRecords` is in an
`internal` package, but `ChannelState` is not.
Other limitations of the evidence above, stated so they are not discovered
later: only the plaintext path was exercised end to end; only a persistent
failure was injected, so the transient case — where the re-read succeeds and
the
failure is attributed to the socket by design — has no measurement behind it;
and the discrimination relies on `read(2)` failing for an unreadable region,
which holds on Linux.
--
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]
