Skip to content

HBASE-28801 WALs are not cleaned even after all entries are flushed #6179

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kiran-maturi wants to merge 3 commits into
base: branch-2
Choose a base branch
from
+9 −4
Open

HBASE-28801 WALs are not cleaned even after all entries are flushed #6179

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2a1f223
HBASE-28801 WALs are not cleaned even after all entries are flushed
Aug 27, 2024
69cdb54
spotless fixes
Aug 27, 2024
17ce08c
added logging for files with unflushed entries
Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions hbase-server/src/main/java/org/apache/hadoop/hbase/regionserver/wal/FSHLog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Contributor

@Apache9 Apache9 Sep 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@kiran-maturi kiran-maturi Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Apache9 Thanks for reviewing.

The highestUnsyncedTxid will only be increased in appendEntry method, so in the doReplaceWriter, after arriving the safe point, the highestUnsyncedTxid will not be changed.

Is it safe to assume that the other threads (calling appendWrite) won't increment highestUnsyncedTxid till the close happens

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.

Yes that is correct accessing in the isUnflushedEntries in the background will cause issues

Copy link
Contributor

@Apache9 Apache9 Sep 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you plan to fix the background close issue or you just want to add some logs in this issue and file new issues for addressing the background close issue?

inflightWALClosures.remove(oldPath.getName());
if (!isUnflushedEntries()) {
markClosedAndClean(oldPath);
}
}
} else {
Writer localWriter = this.writer;
Expand Down
11 changes: 7 additions & 4 deletions hbase-server/src/test/java/org/apache/hadoop/hbase/regionserver/wal/TestFSHLog.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,7 +324,7 @@ public void run() {
/**
* Test for jira https://issues.apache.org/jira/browse/HBASE-28665
*/
public void testWALClosureFailureAndCleanup() throws IOException {
public void testWALClosureFailureAndCleanup() throws IOException, InterruptedException {

class FailingWriter implements WALProvider.Writer {
@Override
Expand Down Expand Up @@ -375,12 +375,15 @@ public void close() throws IOException {
region.put(new Put(b).addColumn(b, b, b));
log.rollWriter();
}
assertEquals(2, log.getClosedErrorCount());
region.put(new Put(b).addColumn(b, b, b));
region.put(new Put(b).addColumn(b, b, b));
region.flush(true);
log.highestUnsyncedTxid = log.highestSyncedTxid.get() + 100;
assertTrue("WAL has unflushed entries ", log.getUnflushedEntriesCount() > 0);
log.rollWriter();
assertEquals("WAL Files not cleaned ", 0, log.walFile2Props.size());
assertEquals("WAL Files not cleaned ", 3, log.walFile2Props.size());
region.flush(true);
log.markClosedAndClean(log.walFile2Props.firstKey());
assertEquals("WAL Files not cleaned ", 1, log.walFile2Props.size());
region.close();
}
}
Expand Down