HDDS-15170. Add mock-based unit tests for DataStream write path by fapifta · Pull Request #10230 · apache/ozone

fapifta · 2026-05-11T01:16:28Z

What changes were proposed in this pull request?

Add MockDatanodePipeline test harness that simulates a Ratis DataStream pipeline using mocked XceiverClientRatis and a concrete DataStreamOutput implementation, with failure injection for chunk writes, putBlock, and watchForCommit
Add TestBlockDataStreamOutput with 12 block-level tests covering sub-chunk writes, exact chunk boundaries, flush boundaries, back-pressure via stream window, hsync happy/error paths, close behavior, write-after-close rejection, ack tracking, data integrity, and putBlock metadata
Add TestKeyDataStreamOutput with 7 key-level tests covering write-and-commit, cross-block boundary allocation, hsync-to-OM integration, container-close retry, multiple hsyncs, and write-after-close rejection

Two block-level tests (hsyncPropagatesIOException, hsyncPropagatesWatchFailure) and one key-level test (hsyncWithBlockErrorDoesNotCallOmHsync) are currently disabled — they document a known hsync exception handling issue where BlockDataStreamOutput.hsync() silently swallows all exceptions via an empty catch block, and KeyDataStreamOutput.handleFlushOrClose() retries hsync failures as if they were write failures. These tests will be enabled when the fix lands.

Generated-by: Claude - Opus 4.6

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-15170

How was this patch tested?

The added unit tests succeed.

Generated-by: Claude - Opus 4.6

adoroszlai · 2026-05-18T09:09:09Z

@fapifta Can you please re-run all checks in your fork https://github.com/fapifta/hadoop-ozone/actions/runs/25658037221? I'd like to compare code coverage before/after the patch. (Unfortunately re-run failed acceptance check works only within 1 day of failure due to artifact expiry.)

fapifta · 2026-05-19T17:38:47Z

@adoroszlai I started it just now. https://github.com/fapifta/hadoop-ozone/actions/runs/25658037221

adoroszlai · 2026-05-19T19:17:37Z

This PR increases coverage of BlockDataStreamOutput from 77% to 81%.

fapifta · 2026-05-31T23:03:37Z

@peterxcli are you planning to review this PR?

It would be nice to merge this one.

peterxcli · 2026-06-01T02:57:25Z

@peterxcli are you planning to review this PR?

It would be nice to merge this one.

sure, please give me some time!

peterxcli · 2026-06-09T09:45:41Z

+    this.watchFailAfter = n;
+    this.watchFailure = err;
+    return this;
+  }


add a builder for fake datanode pipeline?

As currently its constructor has only one parameter, I would consider that as boilerplate bloat... Do you think it is necessary, or is it enough to add a builder if the number of constructor parameters increase further?

I'm ok with the current version, but I have the following structure in my mind:

pipeline = MockDatanodePipeline.newBuilder() .setFailPutBlockAfter(...) .setFailWatchAfter(...) .setFailChunkAfter(...) // or pipeline = MockDatanodePipeline.newBuilder() .setFailChunkAfter(...) // or pipeline = MockDatanodePipeline.newBuilder() .setFailWatchAfter(...) .setFailChunkAfter(...)

but looks like there is no multiple failure injection now, so maybe the builder is unnecessary for now.

I just put this aside yesterday as I wanted to explore the idea, but I realized that the failure setup is a fluid API, so you can do:

MockDatanodePipeline pipeline = new MockDatanodePipeline(blockId) .setFailPutBlockAfter(...) .setFailChunkAfter(...);

peterxcli

thanks @fapifta for introducing the mock pipeline, so we can finally do unit test on data stream output.

left some comments, will review key data stream output test later.

peterxcli · 2026-06-09T17:28:35Z

+    stream.write(ByteBuffer.wrap(data), 0, data.length);
+
+    // hsync should propagate the IOException from the failed putBlock
+    assertThrows(IOException.class, stream::hsync, "hsync() must propagate IOException from failed putBlock");


so should we expose exception from the hysnc?

Yes, that is what is being prepared here, but I wanted to keep things clean, and just added the tests that surfaces the problem that the exception is not exposed.
The tests should be unskipped in a follow up commit that surfaces the exception from hsync itslef, and enable these two tests. These two tests fail now.

The rationale is that if the exception is not exposed, then a client might falsely think that the sync was successful, while it failed, so the client has a different view on what was persisted and what was not.

if the exception is not exposed, then a client might falsely think that the sync was successful, while it failed, so the client has a different view on what was persisted and what was not.

make sense, is there any ticket for this?

I created HDDS-15169 to post a series of changes, the hsync exception exposure comes in the second defined task, though I have not created all of them, it was in flux back then when I created the first two.
HDDS-15225 is the hsync change.

…d a flaky situation surfaced during implementing the changes.

fapifta · 2026-07-06T13:39:58Z

Hi @peterxcli thank you for your thorough review on these test additions. The Fake, now MockDatanodePipeline related suggestions were really great.
I tried to address the review comments even in places where they were not directly added, but a similar comment matched onto the situation elsewhere in the code also. (Like rename to pipeline instead of fake, and explicitly check for the number of operations done.)
I also removed the atomic key creation related test, as that is moved from the stream to S3 in an other change that affected the test.
I found a flakiness with the injected failure and how it surfaces I addressed that also.

I resolved all the relevant comments, except one that I would like to address directly there before we resolve, if you accept my answer there please resolve the review.
Please also re-check the suggested changes once you have the time.

peterxcli

Thanks @fapifta for the update, patch now LGTM.

btw I've replied two of your comments, please also take a look, Thanks!
btw, please update the pr description corespondingly if possible, Thanks!

fapifta · 2026-07-07T07:37:47Z

Thank you for the reviews @adoroszlai and @peterxcli!

* master: (519 commits) HDDS-14544. OM DB Insights: Duplicate API calls triggered when changing limit selector (apache#10677). HDDS-15587. [Recon] Show 0 for offline DN pending deletion instead of -1 (apache#10585). HDDS-15521. StreamBlockInputStream fails with TimeoutIOException without retry or failover. (apache#10479) HDDS-15170. Add mock-based unit tests for DataStream write path (apache#10230) HDDS-15552. Ratis events should not be published as metrics (apache#10523) HDDS-15746. Bump kerby to 2.1.2 (apache#10666) HDDS-15579. Replace SimpleSpanProcessor with BatchSpanProcessor (apache#10569) HDDS-15747. Address review comments for HDDS-15083 (apache#10669) HDDS-15732. Some ozone commands ignore OZONE_MODULE_ACCESS_ARGS (apache#10655) HDDS-15605. Fix flaky testContainerExclusionWithClosedContainerException (apache#10621) HDDS-11855. Fix flaky TestContainerBalancerDatanodeNodeLimit#checkIterationResultException (apache#10667) HDDS-15741. Bump awssdk to 2.46.17 (apache#10661) HDDS-10307. Speed up TestOzoneManagerHAWithStoppedNodes (apache#10658) HDDS-15651. Test case for DiskBalancer when markContainerForDelete fails (apache#10593) HDDS-15742. Bump nimbus-jose-jwt to 10.9.1 (apache#10662) HDDS-15719. Add check for allowed action usage in workflows (apache#10641) HDDS-15744. Bump javassist to 3.32.0-GA (apache#10665) HDDS-15737. Fix intermittent failure in balancerShouldOnlySelectConfiguredIncludeContainers (apache#10660) HDDS-15743. Bump gson to 2.14.0 (apache#10664) HDDS-11093. Fix intermittent failure in TestContainerBalancerDatanodeNodeLimit#testMetrics (apache#10659) ... Conflicts: hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/ozoneimpl/ContainerScanHelper.java

HDDS-15170. Add mock-based unit tests for DataStream write path

cbe154f

Generated-by: Claude - Opus 4.6

fapifta requested review from adoroszlai and szetszwo May 11, 2026 01:16

adoroszlai added AI-gen test labels May 11, 2026

adoroszlai reviewed May 11, 2026

View reviewed changes

Comment thread hadoop-hdds/client/dev-support/findbugsExcludeFile.xml Outdated

Address review comments

f559168

peterxcli requested review from peterxcli May 11, 2026 08:15

peterxcli reviewed Jun 9, 2026

View reviewed changes

github-actions Bot added the stale label Jul 3, 2026

fapifta added 2 commits July 6, 2026 11:55

Merge branch 'master' into HDDS-15170

750c6fb

Address review comments, atomic key creation movement to S3 layer, an…

6354cc8

…d a flaky situation surfaced during implementing the changes.

fapifta added 2 commits July 6, 2026 15:41

Fix checkstyle issues.

1eaf465

Fix checkstyle remove unused imports.

d966396

peterxcli removed the stale label Jul 6, 2026

apache deleted a comment from github-actions Bot Jul 6, 2026

peterxcli approved these changes Jul 6, 2026

View reviewed changes

fapifta merged commit 67fd519 into apache:master Jul 7, 2026
31 checks passed

Uh oh!

Conversation

fapifta commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

Uh oh!

adoroszlai commented May 18, 2026

Uh oh!

fapifta commented May 19, 2026

Uh oh!

adoroszlai commented May 19, 2026

Uh oh!

fapifta commented May 31, 2026

Uh oh!

peterxcli commented Jun 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

peterxcli left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fapifta commented Jul 6, 2026

Uh oh!

peterxcli left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

fapifta commented Jul 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fapifta commented May 11, 2026 •

edited

Loading

peterxcli left a comment •

edited

Loading