Skip to content

HBASE-28665 WALs not marked closed when there are errors in closing WALs #6027

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

Merged
virajjasani merged 5 commits into from
Jul 11, 2024
Merged

HBASE-28665 WALs not marked closed when there are errors in closing WALs #6027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9330bf5
HBASE-28665 WALs not marked closed when there are errors in closing WALs
kiran-maturi Jun 26, 2024
a20ab24
added test case
kiran-maturi Jun 28, 2024
8ee3e40
fixed test cases
Jun 29, 2024
5cbb2f2
fixed log message
Jun 29, 2024
3b29094
spotless fixes
Jun 29, 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
7 changes: 7 additions & 0 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,6 +390,9 @@ protected void doReplaceWriter(Path oldPath, Path newPath, Writer nextWriter) th
closeWriter(this.writer, oldPath, true);
} finally {
inflightWALClosures.remove(oldPath.getName());
if (!isUnflushedEntries()) {
Copy link

@ranganathg ranganathg Jul 2, 2024

Choose a reason for hiding this comment

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

Similar to the code below at line 403, should we also have a catch block to Log a WARNING?
Does it make any difference to first remove old path and then call markCloseAndClean or vice versa? Since the JIRA said we are trying to follow the similar as below, we can try to keep it same?

Copy link

@ranganathg ranganathg Jul 2, 2024

Choose a reason for hiding this comment

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

Just another question, does closeWriter also flush?

Copy link
Contributor Author

@kiran-maturi kiran-maturi Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ranganathg inflightWALClosures.remove(oldPath.getName()); was there previously as well so not changing the behaviour there. We are explicitly looking for the closing for cleanup

Just another question, does closeWriter also flush?

No, this is in the write path it will not trigger any flush

Copy link
Contributor Author

@kiran-maturi kiran-maturi Jul 3, 2024

Choose a reason for hiding this comment

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

@ranganathg sorry I got confused with the flush for region. If you are asking for flush for the entries that are there on the ringbuffer that will be done by the sycn runner threads which will increment the sequenceId

markClosedAndClean(oldPath);
}
}
} else {
Writer localWriter = this.writer;
Expand Down Expand Up @@ -1216,4 +1219,8 @@ Writer getWriter() {
void setWriter(Writer writer) {
this.writer = writer;
}

protected int getClosedErrorCount() {
return closeErrorCount.get();
}
}
65 changes: 65 additions & 0 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 @@ -317,4 +317,69 @@ public void run() {
region.close();
}
}

@Test
/**
* Test for jira https://issues.apache.org/jira/browse/HBASE-28665
*/
public void testWALClosureFailureAndCleanup() throws IOException {

class FailingWriter implements WALProvider.Writer {
@Override
public void sync(boolean forceSync) throws IOException {

}

@Override
public void append(WAL.Entry entry) throws IOException {
}

@Override
public long getLength() {
return 0;
}

@Override
public long getSyncedLength() {
return 0;
}

@Override
public void close() throws IOException {
throw new IOException("WAL close injected failure..");
}
}
final byte[] b = Bytes.toBytes("b");
String name = this.name.getMethodName();
// Have a FSHLog writer implementation that fails during close
try (FSHLog log = new FSHLog(FS, CommonFSUtils.getRootDir(CONF), name,
HConstants.HREGION_OLDLOGDIR_NAME, CONF, null, true, null, null)) {
log.init();

// create a region with the wal
TableDescriptor htd =
TableDescriptorBuilder.newBuilder(TableName.valueOf(this.name.getMethodName()))
.setColumnFamily(ColumnFamilyDescriptorBuilder.of(b)).build();
RegionInfo hri = RegionInfoBuilder.newBuilder(htd.getTableName()).build();
ChunkCreator.initialize(MemStoreLAB.CHUNK_SIZE_DEFAULT, false, 0, 0, 0, null,
MemStoreLAB.INDEX_CHUNK_SIZE_PERCENTAGE_DEFAULT);
final HRegion region = TEST_UTIL.createLocalHRegion(hri, CONF, htd, log);
// Repeat the following steps twice
// * Append writes for the FSHLog
// * Create a new Writer replace the old writer
for (int i = 0; i < 2; i++) {
log.setWriter(new FailingWriter());
region.put(new Put(b).addColumn(b, b, b));
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.rollWriter();
assertEquals("WAL Files not cleaned ", 0, log.walFile2Props.size());
region.close();
}
}
}