PHOENIX-7984 Fence writer on sync failure to prevent false-success RPO loss - #2596
Merged
Merged
Conversation
…O loss A SYNC write failure could partially mutate a block before the durability barrier, then a retry on the same writer would false-succeed -- acking a sync that never reached the peer, a silent RPO loss (S17b). Fence the writer on first append/sync IOException (mirroring HDFS DFSOutputStream single-shot semantics): the first fault is latched and every subsequent append/sync fails fast rather than touching the stream. A fenced writer is never re-driven; recovery is by rotating to a fresh writer (new HDFS pipeline on healthy DataNodes) and replaying the unsynced batch. apply()'s retry obtains that fresh writer through a guarded wait on rotationSignal that re-drives requestRotation() each spin. This fixes the swallowed-request missed-retry: a rotation request coalesced away while a soon-to-complete rotation held the CAS gate is reissued once the gate clears, so a fresh task actually gets scheduled instead of the retry giving up and prematurely downgrading SYNC to STORE_AND_FORWARD. Writer creation stays async on the rotation executor, so the consumer stall is bounded by retryDelayMs and decoupled from standby FS latency. requestRotation() returns a boolean so the waiter exits immediately when rotation is permanently suppressed (failover pending / executor shut down) rather than burning the full budget. Tests: - LogFileWriterSyncTest: writer stays fenced after a sync failure; rejects subsequent append/sync. - testNoSameWriterRetryWhenRotationCannotStageWriter: no second sync on the fenced writer when rotation cannot stage a fresh one; flips to SAF and the unsynced record survives onto the SAF writer. - testRetryReDrivesRotationAfterFirstRotationFails: regression guard for the re-drive -- a first rotation that fails is retried within the budget instead of downgrading to SAF.
…d-path fence - apply(): chain the original write failure as the cause of the InterruptedIOException thrown when the retry wait is interrupted, so the root fault (e.g. peer-DataNode sync failure) survives into failPendingSyncs and crash reports instead of being masked by the interrupt message. - awaitStagedWriter() javadoc: correct the close-promptness claim -- close() does not notify rotationSignal, so a waiter observes close on the next wakeup, bounded by retryDelayMs, not immediately. - LogFileWriterSyncTest.testWriterFencedAfterAppendFailure: guard the append() catch-block fence latch (block-full internal sync failure), which was previously unverified.
apurtell
approved these changes
Aug 14, 2026
apurtell
left a comment
apurtell
left a comment
Contributor
There was a problem hiding this comment.
lgtm, with two minor suggestions
| * actually gets scheduled. The waited-on condition is {@code pendingWriter} itself, so a spurious | ||
| * or unrelated notify just re-checks and loops. Exits immediately when rotation is permanently | ||
| * suppressed (nothing will ever stage). A close is observed on the next wakeup rather than | ||
| * promptly — {@link #close} does not notify {@code rotationSignal} — but the wait is bounded by |
Contributor
There was a problem hiding this comment.
close() could signal rotationSignal, e.g.
public void close(boolean graceful) {
if (!closed.compareAndSet(false, true)) { return; }
synchronized (rotationSignal) { rotationSignal.notifyAll(); } // <--- HERE
stopRotationExecutor();
...
}| * @return the staged writer, or {@code null} if none was staged before the deadline / close / | ||
| * permanent suppression. The caller drains it via {@link #checkAndReplaceWriter}. | ||
| */ | ||
| private LogFileWriter awaitStagedWriter() throws InterruptedIOException { |
Contributor
There was a problem hiding this comment.
If a rotation fails fast the loop can spin through all rotations in milliseconds and close the log before the SAF downgrade even gets a chance to help. Consider a tiny back-off e.g. min(remainingMs, 50) on the first spin, or a fraction of retryDelayMs between spins.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A SYNC write failure could partially mutate a block before the durability barrier, then a retry on the same writer would false-succeed -- acking a sync that never reached the peer, a silent RPO loss.
Fence the writer on first append/sync IOException (mirroring HDFS DFSOutputStream single-shot semantics): the first fault is latched and every subsequent append/sync fails fast rather than touching the stream. A fenced writer is never re-driven; recovery is by rotating to a fresh writer (new HDFS pipeline on healthy DataNodes) and replaying the unsynced batch.
apply()'s retry obtains that fresh writer through a guarded wait on rotationSignal that re-drives requestRotation() each spin. This fixes the swallowed-request missed-retry: a rotation request coalesced away while a soon-to-complete rotation held the CAS gate is reissued once the gate clears, so a fresh task actually gets scheduled instead of the retry giving up and prematurely downgrading SYNC to STORE_AND_FORWARD. Writer creation stays async on the rotation executor, so the consumer stall is bounded by retryDelayMs and decoupled from standby FS latency. requestRotation() returns a boolean so the waiter exits immediately when rotation is permanently suppressed (failover pending / executor shut down) rather than burning the full budget.
Tests: