yandrey321 commented on code in PR #11783:
URL: https://github.com/apache/ozone/pull/11783#discussion_r4427326523
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/ozone/client/io/ECBlockInputStream.java:
##########
@@ -327,15 +335,56 @@ protected long remaining() {
return blockLength() - position;
}
+ /**
+ * Reads the cells covered by the buffer from the internal block streams of
their data indexes with positioned
+ * reads, so neither this stream nor its internal streams change position,
and calls may run concurrently with each
+ * other and with sequential reads. The indexes are read in parallel if the
parallel read executor is set.
+ *
+ * <p>A failed index is not switched to a spare. Instead, a {@link
BadDataLocationException} carrying every failed
+ * index is thrown, so that the caller can fall back to a reconstruction
read for this call.
+ */
@Override
- protected int readPositioned(long offset, ByteBuffer buffer) {
- throw new UnsupportedOperationException("Use ECBlockInputStreamProxy for
positioned reads");
- }
+ protected int readPositioned(long offset, ByteBuffer buffer) throws
IOException {
+ checkOpen();
Review Comment:
Yes, and it's more than a nit: it closes a small race. `readPositioned`
isn't `synchronized(this)`, so it can run
alongside `close()`:
1. A pread passes the first `checkOpen()`.
2. `close()` sets `closed = true`, then `closeStreams()` closes each
per-index stream and sets its slot to null.
3. The pread calls `getOrOpenStream(index)`, finds the slot empty, and opens
a **new** stream.
The read itself still fails with an `IOException` on the `checkOpen()` after
`runIndexReads`. But the new stream stays in `blockStreams[index]` after
`close()` has finished. Its read has gone through
`ChunkInputStream.acquireClient()`, so it holds an `XceiverClient` reference
that nothing releases.
Checking under `streamLock` closes that window, because `close()` sets
`closed` before `closeStreams()` takes `streamLock` for each index. Either the
pread sees `closed` and throws, or `closeStream(index)` runs after the pread
released the lock and closes the stream it opened. I've put the check in
`getOrOpenStream()` itself, under `streamLock`, so every caller is covered: the
sequential paths already hold `this`, which `close()` also takes, so for them
it costs nothing. The first `checkOpen()` stays as an early exit, and the one
after the reads stays for the reason in its comment.
`testOpeningIndexStreamAfterCloseFails` covers it. It puts the stream in the
state a racing pread sees, closed after its first check, then opens an index
stream: that now fails with "Stream is closed" and opens no new stream. Without
the change, it opens one.
--
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]
