peterxcli commented on code in PR #11143:
URL: https://github.com/apache/ozone/pull/11143#discussion_r3880856866
##########
hadoop-hdds/client/src/main/java/org/apache/hadoop/hdds/scm/storage/BlockDataStreamOutput.java:
##########
@@ -541,8 +541,13 @@ public void hsync() throws IOException {
if (!isClosed()) {
handleFlush(false);
}
- } catch (Exception e) {
-
+ } catch (IOException e) {
+ throw e;
+ } catch (InterruptedException e) {
+ Thread.currentThread().interrupt();
+ handleInterruptedException(e, false);
+ } catch (ExecutionException | RuntimeException e) {
+ handleExecutionException(e);
Review Comment:
Two things here:
**`catch (IOException e) { throw e; }` is redundant** — `handleFlush`
declares `IOException`, so it propagates on its own. `close()` twelve lines
below (`:611`) already has exactly the right shape: only `ExecutionException`
and `InterruptedException`.
**`| RuntimeException` looks over-broad and unnecessary.** I removed it and
all 14 tests in `TestBlockDataStreamOutput` still pass, so nothing in this
class's test coverage needs it. It also funnels programming errors (NPE,
`Preconditions` failures) through `setIoException`, which poisons the stream's
`ioException` field so every subsequent call fails with a misleading
`Unexpected Storage Container Exception: java.lang.NullPointerException`. If a
specific runtime failure motivated adding it in the "Fix the test failure"
commit, could you say which one? Otherwise I'd drop it.
```suggestion
} catch (ExecutionException e) {
handleExecutionException(e);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
handleInterruptedException(e, false);
```
##########
hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockDataStreamOutput.java:
##########
@@ -171,10 +171,10 @@ void hsyncPropagatesIOException() throws Exception {
// hsync should propagate the IOException from the failed putBlock
assertThrows(IOException.class, stream::hsync, "hsync() must propagate
IOException from failed putBlock");
- stream.close();
+ stream.cleanup(false);
Review Comment:
Swapping `close()` for `cleanup(false)` sidesteps a contract worth pinning.
`close()` after a failed hsync throws, correctly:
```
java.io.IOException: Unexpected Storage Container Exception:
java.util.concurrent.CompletionException: ... java.io.IOException: simulated
putBlock fail
```
That is exactly the "the caller must not think the sync succeeded" property
this PR is about, so I'd assert it rather than route around it. `close()` still
releases the client through its own `finally { cleanup(false); }`, so nothing
leaks. Verified green.
```suggestion
assertThrows(IOException.class, stream::close);
```
##########
hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/io/KeyDataStreamOutput.java:
##########
@@ -389,6 +389,9 @@ private void handleFlushOrClose(StreamAction op) throws
IOException {
try {
handleStreamAction(entry, op);
} catch (IOException ioe) {
+ if (op == StreamAction.HSYNC) {
+ throw ioe;
+ }
Review Comment:
This is my main concern, and it's outside HDDS-15225's stated scope (the
Jira is scoped to `BlockDataStreamOutput.hsync()`; this hunk arrived only in
the "Fix the test failure" commit).
**It inverts the Ratis behavior at the key level.**
`KeyOutputStream.handleFlushOrClose` (`KeyOutputStream.java:585`) routes HSYNC
failures through `handleException(entry, ioe, false)` and retries exactly like
writes — no HSYNC special case. The PR description says this "aligns the
DataStream hsync with the Ratis behavior", which is true of the block-level fix
but the opposite of true here.
**The recovery it disables genuinely works.** With this hunk reverted, on
the two-pipeline test added in the same commit, I instrumented the run:
```
p2 received=200 putBlocks=1 watches=1
```
`handleException` excludes the failed pipeline, allocates a new block,
replays the 200 buffered bytes (retained in `bufferList` until acked), commits
them with a successful putBlock + watchForCommit, and only then calls
`hsyncKey` once. The `hsyncKey` is honest.
**Practical cost:** `CLOSED_CONTAINER_IO` is routine, and
`containerCloseTriggersRetryOnNewBlock` in this test class asserts the
DataStream path recovers from it. With this hunk, the same event during
`hsync()` becomes a hard failure instead of a transparent retry.
**Second-order effect:** because the enclosing `catch (Exception e) {
markStreamClosed(); throw e; }` at `:400` now fires on the first IOException,
one failed hsync permanently closes the whole stream. I probed what `close()`
does next:
```
close() after failed hsync returned normally
commitKey was NEVER called after close()
```
`close()` hits `if (closed) return;` and reports success without committing
the key — so `try { write; hsync } finally { close }` swallows the failure
entirely.
I'd drop this hunk. All three previously-skipped tests pass without it
(verified). If the fail-fast semantics are wanted, they seem worth their own
subtask under HDDS-15169 with the retry-vs-fail-fast tradeoff written down.
```suggestion
```
##########
hadoop-hdds/client/src/test/java/org/apache/hadoop/hdds/scm/storage/TestBlockDataStreamOutput.java:
##########
@@ -187,7 +187,7 @@ void hsyncPropagatesWatchFailure() throws Exception {
// hsync should propagate the watch failure
assertThrows(IOException.class, stream::hsync, "hsync() must propagate
IOException from failed watchForCommit");
- stream.close();
+ stream.cleanup(false);
Review Comment:
Same as above — asserting that `close()` throws documents the post-failure
contract, and `close()`'s own `finally` still calls `cleanup(false)`.
```suggestion
assertThrows(IOException.class, stream::close);
```
##########
hadoop-ozone/client/src/test/java/org/apache/hadoop/ozone/client/io/TestKeyDataStreamOutput.java:
##########
@@ -227,15 +227,14 @@ void hsyncCallsOmHsyncKey() throws Exception {
}
}
-// @Test - skipped as it fails now
+ @Test
void hsyncWithBlockErrorDoesNotCallOmHsync() throws Exception {
- MockDatanodePipeline pipeline = new MockDatanodePipeline();
+ MockDatanodePipeline pipeline1 = new MockDatanodePipeline(new BlockID(1,
1));
+ MockDatanodePipeline pipeline2 = new MockDatanodePipeline(new BlockID(2,
2));
// First putBlock will fail
- pipeline.failPutBlockAfter(0, () -> new IOException("putBlock failed"));
-
- OzoneManagerProtocol omClient = createOmClient(pipeline);
-
- KeyDataStreamOutput stream = createKeyStream(omClient, pipeline);
+ pipeline1.failPutBlockAfter(0, () -> new IOException("putBlock failed"));
+ OzoneManagerProtocol omClient = createOmClient(pipeline1, pipeline2);
+ KeyDataStreamOutput stream = createKeyStream(omClient, pipeline1,
pipeline2);
Review Comment:
This rewrite is what made the `KeyDataStreamOutput` change necessary. The
original used one pipeline whose putBlock always fails, so retries genuinely
exhaust and hsync throws. Adding a **healthy** `pipeline2` makes recovery
possible, which then required the production change to keep `verify(omClient,
never()).hsyncKey(...)` true.
With the original body restored and only the `BlockDataStreamOutput.hsync()`
fix applied, all 7 tests in this class pass.
Worth settling explicitly: should the assertion be *"hsync never calls
hsyncKey after a block error"*, or *"hsync doesn't call hsyncKey until the data
is durable somewhere"*? The Ratis path implements the second.
Minor, while here: the trailing `stream.close()` at `:246` is dead under the
current patch (`closed` is already `true` after `markStreamClosed()`), and the
stream isn't in try-with-resources, so it leaks if an assertion fails.
```suggestion
MockDatanodePipeline pipeline = new MockDatanodePipeline();
// First putBlock will fail
pipeline.failPutBlockAfter(0, () -> new IOException("putBlock failed"));
OzoneManagerProtocol omClient = createOmClient(pipeline);
KeyDataStreamOutput stream = createKeyStream(omClient, pipeline);
```
--
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]
