Apache9 commented on code in PR #6179:
URL: https://github.com/apache/hbase/pull/6179#discussion_r1745677591
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java:
##########
@@ -390,10 +390,10 @@ protected void doReplaceWriter(Path oldPath, Path
newPath, Writer nextWriter) th
try {
closeWriter(this.writer, oldPath, true);
} finally {
+ // closing this as there is no other chance we can set close to
true
+ // during clean up we check for unflushed entries
+ markClosedAndClean(oldPath);
Review Comment:
Take a look at the code again, the `isUnflushedEntries` should work as
expected. The `highestUnsyncedTxid` will only be increased in appendEntry
method, so in the `doReplaceWriter`, after arriving the safe point, the
`highestUnsyncedTxid` will not be changed.
I think there is another problem that in closeWriter, we will also check
isUnflushedEntries and also increase the closeErrorCount, which may affect the
logic here. But for normal case, closeWriter is executed in a background thread
pool, so it is not safe to call isUnflushedEntries as it does not reflect the
real state before closing... And why we put the close in background is that,
even if it fails, it is not a big deal as we can make sure that all entries
have already been flushed, a close failure only means we may fail to write the
trailer.
I think here, we should only increase the closeErrorCount when closing in
foreground, and also, when calling closeWriter, we should pass the result of
isUnflushedEntries in, instead of calling it everytime when we want to check
this state.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
