Apache9 commented on a change in pull request #762: HBASE-23157 WAL unflushed
seqId tracking may wrong when Durability.AS…
URL: https://github.com/apache/hbase/pull/762#discussion_r339374783
##########
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/SequenceIdAccounting.java
##########
@@ -356,9 +356,36 @@ Long startCacheFlush(final byte[] encodedRegionName,
final Map<byte[], Long> fam
return lowestUnflushedInRegion;
}
- void completeCacheFlush(final byte[] encodedRegionName) {
+ void completeCacheFlush(byte[] encodedRegionName, long maxFlushedSeqId) {
+ // This is a simple hack to avoid maxFlushedSeqId go backwards.
+ // The system works fine normally, but if we make use of
Durability.ASYNC_WAL and we are going
+ // to flush all the stores, the maxFlushedSeqId will be next seq id of the
region, but we may
+ // still have some unsynced WAL entries in the ringbuffer after we call
startCacheFlush, and
+ // then it will be recorded as the lowestUnflushedSeqId by the above
update method, which is
+ // less than the current maxFlushedSeqId. And if next time we only flush
the family with this
+ // unusual lowestUnflushedSeqId, the maxFlushedSeqId will go backwards.
+ // This is an unexpected behavior so we should fix it, otherwise it may
cause unexpected
+ // behavior in other area and then cause data loss maybe.
+ // The solution here is a bit hack but fine. Just replace the
lowestUnflushedSeqId with
+ // maxFlushedSeqId + 1 if it is lesser. Durability.ASYNC_WAL means we do
not care data loss for
+ // this record, so it is fine to just bump the seq id, to avoid
introducing data loss for other
+ // records(maybe).
+ // Use +1 is fine here. If the maxFlushesSeqId is just the flushOpSeqId,
it means we have
+ // flushed all the stores so the seq id for actual data should be at least
plus 1. And if we do
+ // not flush all the stores, then the maxFlushedSeqId is calculateld by
lowestUnflushedSeqId -
+ // 1, so here let's plus the 1 back.
+ Long wrappedSeqId = Long.valueOf(maxFlushedSeqId + 1);
synchronized (tieLock) {
this.flushingSequenceIds.remove(encodedRegionName);
+ Map<ImmutableByteArray, Long> unflushed =
lowestUnflushedSequenceIds.get(encodedRegionName);
+ if (unflushed == null) {
+ return;
Review comment:
There is a wal sync after startCacheFlush so this will not happen, the only
window where we can add smaller seqId is between we call startCacheFlush and
the later wal sync.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
