HBASE-30341: Fix FSHLog WAL lockup on uncaught exception in publishSyncOnRingBuffer - #8611
Open
sidkhillon wants to merge 3 commits into
Open
sidkhillon wants to merge 3 commits into
sidkhillon wants to merge 3 commits into
Conversation
…ncOnRingBuffer (not yet upstream) publishSyncOnRingBuffer claims a Disruptor sequence via RingBuffer.next() and previously called getSyncFuture() before the try whose finally publishes the sequence. An exception from getSyncFuture (seen in production as an NPE from a corrupted Guava cache in SyncFutureCache) left the claimed slot unpublished, so the consumer could never advance and the whole WAL deadlocked. - Move getSyncFuture inside the try so the sequence is always published. - Make the RingBufferEventHandler tolerate the resulting empty truck by logging and falling through (matching master's AbstractFSWAL.consume) rather than failing outstanding syncs. - Make SyncFutureCache.getIfPresentOrNew non-throwing: the cache is purely an allocation optimisation, so fall back to a new SyncFuture if it throws. Adds a regression test for the WAL lockup and a test for the SyncFutureCache fallback. The FSHLog parts were incidentally fixed on master by HBASE-27231, never backported. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…reCache test Guava's Cache is annotated @DoNotMock, so Mockito.mock(Cache.class) fails the errorProne compile that Yetus runs. Fake it with a ForwardingCache subclass whose asMap() throws instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
sidkhillon
force-pushed
the
HBASE-30341-fshlog-wal-lockup-branch-2
branch
from
September 3, 2026 15:42
c181ff4 to
f4ffa5f
Compare
sidkhillon
marked this pull request as ready for review
September 3, 2026 18:44
hgromer
reviewed
Sep 9, 2026
| cleanupOutstandingSyncsOnException(sequence, | ||
| new IllegalStateException("Neither append nor sync")); | ||
| // Return to keep processing. | ||
| return; |
Contributor
There was a problem hiding this comment.
Should we keep the return here? I'm pretty sure falling through isn't problematic, but it might be cleaner/easier to reason about if we simply returned here.
Contributor
Author
There was a problem hiding this comment.
Agreed, it should be the same but more readable if we return
hgromer
reviewed
Sep 9, 2026
| return; | ||
| // Empty truck: publishSyncOnRingBuffer claimed the sequence but threw before loading it. | ||
| // Fall through rather than failing syncs, so the empty slot is harmless. | ||
| LOG.warn("RingBufferTruck with unexpected type: {}", truck.type()); |
Contributor
There was a problem hiding this comment.
Would it be better to have an explicit case for this? Maybe something like
} else if (truck.type() == RingBufferTruck.Type.EMPTY) {
LOG.warn("Empty RingBufferTruck at sequence {}", sequence);
return;
} else {
cleanupOutstandingSyncsOnException(sequence,
new IllegalStateException("Unexpected truck type: " + truck.type()));
return;
}That way we still cleanup on truly exceptional cases?
Contributor
Author
There was a problem hiding this comment.
Sure! Just took your suggestion
Per review, use an explicit RingBufferTruck.Type.EMPTY case that logs and returns, keeping the cleanup path for any other truck type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
This looks good to me, I've added Chenglei and Duo as reviewers in case they'd like to comment. |
Apache9
reviewed
Sep 16, 2026
| // it even if this throws, else the consumer wedges. | ||
| SyncFuture syncFuture = null; | ||
| try { | ||
| syncFuture = getSyncFuture(sequence, forceSync); |
Contributor
There was a problem hiding this comment.
What exception will getSyncFuture throw? Seems the method only has memory operations...
Contributor
Author
There was a problem hiding this comment.
It's all in-memory, but the memory it mutates is Guava's LocalCache, and its write-order queue (present because SyncFutureCache uses expireAfterWrite) can NPE under a race. We hit it in production on branch-2.6 (hbase.wal.provider=filesystem):
java.lang.NullPointerException: Cannot invoke "org.apache.hbase.thirdparty.com.google.common.cache.ReferenceEntry.setNextInWriteQueue(org.apache.hbase.thirdparty.com.google.common.cache.ReferenceEntry)" because "previous" is null
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache.connectWriteOrder(LocalCache.java:1818)
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache$WriteQueue.remove(LocalCache.java:3725)
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache$Segment.removeValueFromChain(LocalCache.java:3249)
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache$Segment.remove(LocalCache.java:3079)
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache.remove(LocalCache.java:4273)
at org.apache.hadoop.hbase.regionserver.wal.SyncFutureCache.getIfPresentOrNew(SyncFutureCache.java:61)
at org.apache.hadoop.hbase.regionserver.wal.AbstractFSWAL.getSyncFuture(AbstractFSWAL.java:1093)
at org.apache.hadoop.hbase.regionserver.wal.FSHLog.publishSyncOnRingBuffer(FSHLog.java:789)
at org.apache.hadoop.hbase.regionserver.wal.FSHLog.publishSyncOnRingBuffer(FSHLog.java:784)
at org.apache.hadoop.hbase.regionserver.wal.FSHLog.publishSyncThenBlockOnCompletion(FSHLog.java:801)
at org.apache.hadoop.hbase.regionserver.wal.FSHLog.doSync(FSHLog.java:836)
at org.apache.hadoop.hbase.regionserver.wal.AbstractFSWAL.sync(AbstractFSWAL.java:605)
at org.apache.hadoop.hbase.regionserver.HRegion.doWALAppend(HRegion.java:7956)
at org.apache.hadoop.hbase.regionserver.HRegion.batchMutate(HRegion.java:4597)
... RSRpcServices.multi -> RpcServer.call -> CallRunner.run -> RpcHandler.run
| // Invalidate the entry if a mapping exists. We do not want it to be reused at the same time. | ||
| SyncFuture future = syncFutureCache.asMap().remove(Thread.currentThread()); | ||
| return (future == null) ? new SyncFuture() : future; | ||
| } catch (RuntimeException e) { |
Contributor
There was a problem hiding this comment.
Where does the RuntimeException come from?
Contributor
Author
There was a problem hiding this comment.
There can be the same NPE from Guava's LocalCache write-queue
java.lang.NullPointerException: Cannot invoke "...ReferenceEntry.setNextInWriteQueue(...)" because "previous" is null
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache.connectWriteOrder(LocalCache.java:1818)
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache$WriteQueue.remove(LocalCache.java:3725)
at org.apache.hbase.thirdparty.com.google.common.cache.LocalCache.remove(LocalCache.java:4273)
at org.apache.hadoop.hbase.regionserver.wal.SyncFutureCache.getIfPresentOrNew(SyncFutureCache.java:61)I'm happy to reduce this to just a NPE catch if you'd like. I kept it broad so any exception falls back to a new SyncFuture()
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
3 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.
publishSyncOnRingBufferclaims a Disruptor sequence via RingBuffer.next() and previously called getSyncFuture() before the try whose finally publishes the sequence. An exception from getSyncFuture (seen in production as an NPE from a corrupted Guava cache in SyncFutureCache) left the claimed slot unpublished, so the consumer could never advance and the whole WAL deadlocked.Adds a regression test for the WAL lockup and a test for the SyncFutureCache fallback. The FSHLog parts were incidentally fixed on master by HBASE-27231, never backported.