[
https://issues.apache.org/jira/browse/HDDS-17667?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
Siyao Meng updated HDDS-17667:
------------------------------
Attachment: TestBugCR2FailedWatchStaleFuture.java
> Hardening: AbstractCommitWatcher keeps the cached future of a failed watch
> for commit uncompleted, so a repeated watch of that index would hang
> -----------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: HDDS-17667
> URL: https://issues.apache.org/jira/browse/HDDS-17667
> Project: Apache Ozone
> Issue Type: Bug
> Affects Versions: 1.4.0, 2.0.0, 1.4.1, 2.1.0, 2.2.0, 2.1.1, 2.1.2, 2.2.1,
> 2.2.2
> Reporter: Siyao Meng
> Priority: Minor
> Attachments: CR-2-complete-cached-watch-future-on-failure.patch,
> TestBugCR2FailedWatchStaleFuture.java
>
>
> h3. Mechanism
> {{AbstractCommitWatcher.watchForCommitAsync}} caches one future per log index
> in {{replies}} so that concurrent watches of the same index share one
> request. Only the success path completes the cached future and removes it.
> When the client's watch fails, the future returned to the first caller fails,
> but the cached future stays in {{replies}} and is never completed. A
> concurrent caller that joined it, or any later watch of the same index on the
> same watcher, gets that future and waits on it (untimed). The later watch
> never reaches the datanode client. {{cleanup()}} clears the map but does not
> complete the future. The gap exists since the cache was added in HDDS-2146
> (synchronous then, with {{join()}}), and HDDS-11174 kept it.
> h3. Trigger
> None through a supported write path today; the cache is never hit there.
> * Ratis keys: {{BlockOutputStream.watchForCommit}} is chained once to each
> PutBlock, and each PutBlock has its own Raft log index. An hsync or flush
> without new data reuses the last flush future and does not call the watcher.
> After a failed watch the block stream keeps the error, so later hsync, write
> and close calls fail in {{checkOpen()}} before any watch.
> * Streaming keys: {{watchOnFirstIndex}} and {{watchOnLastIndex}} run on the
> writer thread. Two callers could only join one pending watch if two threads
> used one {{KeyDataStreamOutput}} at once, which is not a supported usage (it
> has no synchronization). The only index that repeats is 0, used for buffers
> of a PutBlock sent without Raft, and {{XceiverClientRatis.watchForCommit(0)}}
> completes at once without failing.
> h3. Impact
> No user visible effect today. This is a latent defect: if a future caller
> watched an index twice on one watcher (for example two threads on one
> streaming key), it would hang forever after a failed watch instead of seeing
> the error.
> h3. Reproduction
> PASS on unmodified source, deterministic, no cluster.
> [^TestBugCR2FailedWatchStaleFuture.java] ({{hadoop-hdds/client}}) uses a stub
> datanode client whose watches fail.
> * {{failedWatchLeavesStaleFutureForSameIndex}} calls {{CommitWatcher}}
> directly: after the watch for index 7 fails, the joined future and a later
> watch of index 7 (the same object) are not completed, and the client received
> one watch request.
> * {{failedStreamDoesNotWatchSameIndexAgain}} goes through the public
> {{RatisBlockOutputStream}} API and shows why it is not reached: after the
> failed hsync, a second hsync, a write and {{close()}} throw the saved error
> and send no watch.
> A passing test means the defect is present.
> h3. Patch
> [^CR-2-complete-cached-watch-future-on-failure.patch], against
> ec5ed5f7ea0e4b406aee53aae4906616819cba2b. It also applies to master at
> b683fd3f294.
> When the client's watch future fails, {{watchForCommitAsync}} removes the
> cached future with a conditional {{remove(commitIndex, f)}} and completes it
> exceptionally with the watch error, so joined callers fail with that error
> and a later watch sends a new request. The success path and the future
> returned to the first caller are unchanged. Covered by
> {{testFailedWatchForCommitCompletesCachedFuture}} in the existing
> {{TestBlockOutputStreamCorrectness}}, which checks that the joined future
> fails with the watch error and that a later watch is sent again. It fails
> without the change because the joined future stays incomplete. With it, the
> hdds-client storage and ozone-client unit tests and the mini cluster suites
> {{TestCommitWatcher}} and {{TestBlockOutputStreamWithFailures}} (22 tests)
> pass, and checkstyle is clean. With the patch the first reproduction test
> fails (all three futures complete exceptionally and the client receives two
> requests). The second still passes.
> Since the cache is never hit on a supported path, removing it would be the
> alternative. This patch keeps the existing design and only makes its failure
> path correct.
> Related: HDDS-17648 (a watch future that never completes when the Ratis
> client is closed), still under review.
> Found by code review of the Ozone client Ratis block write and failure
> handling paths, as part of the TLA+ verification effort under HDDS-15926, on
> commit ec5ed5f7ea0e4b406aee53aae4906616819cba2b. Checked against HDDS issues
> and apache/ozone pull requests for duplicates before filing. The attached
> patch is a proposal for review. Generated with Specula (Claude Opus 5.5).
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
