Skip to content

Commit 91c4a33

Browse files
void-ptr974void-ptr974
committed
Fix compacted entry log recovery filename
1 parent cbb3367 commit 91c4a33

2 files changed

Lines changed: 226 additions & 1 deletion

File tree

‎bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/DefaultEntryLogger.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1470,7 +1470,8 @@ public Collection<CompactionEntryLog> incompleteCompactionLogs() {
14701470
}
14711471
continue;
14721472
}
1473-
File finalLogFile = new File(compactedFile.getParentFile(), compactionLogId + ".log");
1473+
File finalLogFile = new File(compactedFile.getParentFile(),
1474+
logId2HexString(compactionLogId) + ".log");
14741475

14751476
compactionLogs.add(
14761477
new EntryLoggerCompactionEntryLog(compactionLogId, compactedLogId,

‎bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/DefaultEntryLogTest.java‎

Lines changed: 224 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import java.nio.channels.FileChannel;
4949
import java.nio.charset.Charset;
5050
import java.util.ArrayList;
51+
import java.util.Collection;
5152
import java.util.HashSet;
5253
import java.util.LinkedList;
5354
import java.util.List;
@@ -65,11 +66,14 @@
6566
import java.util.concurrent.locks.Lock;
6667
import org.apache.bookkeeper.bookie.DefaultEntryLogger.BufferedLogChannel;
6768
import org.apache.bookkeeper.bookie.LedgerDirsManager.NoWritableLedgerDirException;
69+
import org.apache.bookkeeper.bookie.storage.CompactionEntryLog;
70+
import org.apache.bookkeeper.bookie.storage.EntryLogScanner;
6871
import org.apache.bookkeeper.common.testing.annotations.FlakyTest;
6972
import org.apache.bookkeeper.conf.ServerConfiguration;
7073
import org.apache.bookkeeper.conf.TestBKConfiguration;
7174
import org.apache.bookkeeper.test.TestStatsProvider;
7275
import org.apache.bookkeeper.util.DiskChecker;
76+
import org.apache.bookkeeper.util.HardLink;
7377
import org.apache.bookkeeper.util.IOUtils;
7478
import org.apache.bookkeeper.util.collections.ConcurrentLongLongHashMap;
7579
import org.apache.commons.io.FileUtils;
@@ -983,6 +987,226 @@ public void testReadEntryWithoutLedgerID() throws Exception {
983987
}
984988
}
985989

990+
@Test
991+
public void testRecoverCompactedLogUsesHexFinalLogFileNameForLogIdTen() throws Exception {
992+
// The next compaction log id is previousLogId + 1. Log id 10 is named "a.log",
993+
// so recovery must not rebuild the final log file as the decimal name "10.log".
994+
verifyRecoveredCompactionLogUsesExpectedFinalLogFileName(9, "a.log", "10.log");
995+
}
996+
997+
@Test
998+
public void testRecoverCompactedLogUsesHexFinalLogFileNameForLogIdSixteen() throws Exception {
999+
// Log id 16 is named "10.log". This covers a numeric-looking hex name that
1000+
// was previously rebuilt as the decimal name "16.log" during recovery.
1001+
verifyRecoveredCompactionLogUsesExpectedFinalLogFileName(15, "10.log", "16.log");
1002+
}
1003+
1004+
@Test
1005+
public void testRecoverCompactedLogUsesHexFinalLogFileNameForLargerLogId() throws Exception {
1006+
// Use a larger id to make the decimal/hex mismatch obvious: 500 decimal is
1007+
// "1f4" in the entry log file name.
1008+
verifyRecoveredCompactionLogUsesExpectedFinalLogFileName(499, "1f4.log", "500.log");
1009+
}
1010+
1011+
@Test
1012+
public void testRecoverCompactedLogWithStaleDecimalLogFileFromOldRecovery() throws Exception {
1013+
long sourceLogId = 1L;
1014+
long previousLogId = 499L;
1015+
long expectedCompactionLogId = previousLogId + 1;
1016+
String expectedFinalLogFileName = "1f4.log";
1017+
String staleDecimalLogFileName = "500.log";
1018+
long ledgerId = 1234L;
1019+
long entryId = 5L;
1020+
ByteBuf entry = generateEntry(ledgerId, entryId);
1021+
1022+
// Simulate an upgrade from a version that already attempted the buggy
1023+
// recovery. That old recovery could create "500.log" before failing to
1024+
// scan log id 500, while the ".compacted" marker remained on disk.
1025+
resetEntryLoggerWithLastLogId(previousLogId);
1026+
1027+
CompactionEntryLog compactionLog = entryLogger.newCompactionLog(sourceLogId);
1028+
assertEquals(expectedCompactionLogId, compactionLog.getDstLogId());
1029+
1030+
try {
1031+
compactionLog.addEntry(ledgerId, entry);
1032+
compactionLog.flush();
1033+
compactionLog.markCompacted();
1034+
1035+
File compactedFile = new File(curDir, expectedFinalLogFileName + "."
1036+
+ DefaultEntryLogger.logId2HexString(sourceLogId)
1037+
+ TransactionalEntryLogCompactor.COMPACTED_SUFFIX);
1038+
File expectedFinalLogFile = new File(curDir, expectedFinalLogFileName);
1039+
File staleDecimalLogFile = new File(curDir, staleDecimalLogFileName);
1040+
assertTrue(compactedFile.exists(), "Compacted recovery marker should exist");
1041+
assertFalse(expectedFinalLogFile.exists(), "Expected hex final log should not exist before recovery");
1042+
assertFalse(staleDecimalLogFile.exists(), "Stale decimal log should not exist before simulation");
1043+
1044+
// This hard link is the on-disk artifact left by the old buggy
1045+
// makeAvailable(): it points at the compacted content but has the
1046+
// decimal file name that scanEntryLog(500) will not use.
1047+
HardLink.createHardLink(compactedFile, staleDecimalLogFile);
1048+
assertTrue(staleDecimalLogFile.exists(), "Stale decimal log should exist before fixed recovery");
1049+
1050+
Collection<CompactionEntryLog> incompleteLogs = entryLogger.incompleteCompactionLogs();
1051+
assertEquals(1, incompleteLogs.size());
1052+
1053+
CompactionEntryLog recoveredLog = incompleteLogs.iterator().next();
1054+
assertEquals(expectedCompactionLogId, recoveredLog.getDstLogId());
1055+
assertEquals(sourceLogId, recoveredLog.getSrcLogId());
1056+
1057+
// Fixed recovery should ignore the stale decimal log and still create
1058+
// the expected hex-named final log used by scanEntryLog(500).
1059+
recoveredLog.makeAvailable();
1060+
assertTrue(expectedFinalLogFile.exists(), "Fixed recovery should create the hex-named final log");
1061+
assertTrue(staleDecimalLogFile.exists(), "Fixed recovery should leave the old stale file untouched");
1062+
1063+
assertCompactionLogContainsEntry(recoveredLog, ledgerId, entryId);
1064+
recoveredLog.finalizeAndCleanup();
1065+
assertFalse(compactedFile.exists(), "Successful recovery cleanup should remove the compacted marker");
1066+
assertTrue(staleDecimalLogFile.exists(), "Cleanup should not remove unrelated stale decimal log files");
1067+
} finally {
1068+
ReferenceCountUtil.release(entry);
1069+
}
1070+
}
1071+
1072+
@Test
1073+
public void testRecoverCompactedLogKeepsSingleDigitFinalLogFileName() throws Exception {
1074+
// Single-digit ids have the same decimal and hex file name. Keep this case to
1075+
// show the recovery path remains compatible when the old and new names match.
1076+
verifyRecoveredCompactionLogUsesExpectedFinalLogFileName(8, "9.log", null);
1077+
}
1078+
1079+
@Test
1080+
public void testNewCompactionLogKeepsHexFinalLogFileName() throws Exception {
1081+
long sourceLogId = 1L;
1082+
long previousLogId = 499L;
1083+
long expectedCompactionLogId = previousLogId + 1;
1084+
String expectedFinalLogFileName = "1f4.log";
1085+
long ledgerId = 1234L;
1086+
long entryId = 5L;
1087+
ByteBuf entry = generateEntry(ledgerId, entryId);
1088+
1089+
resetEntryLoggerWithLastLogId(previousLogId);
1090+
1091+
// This is the normal, non-recovery compaction path. finalLogFile is derived
1092+
// from the current compaction log file name, so it should already be hex.
1093+
CompactionEntryLog compactionLog = entryLogger.newCompactionLog(sourceLogId);
1094+
assertEquals(expectedCompactionLogId, compactionLog.getDstLogId());
1095+
1096+
try {
1097+
compactionLog.addEntry(ledgerId, entry);
1098+
compactionLog.flush();
1099+
// markCompacted() leaves a recovery marker such as
1100+
// "1f4.log.1.compacted" and removes the temporary compacting file.
1101+
compactionLog.markCompacted();
1102+
1103+
File compactedFile = new File(curDir, expectedFinalLogFileName + "."
1104+
+ DefaultEntryLogger.logId2HexString(sourceLogId)
1105+
+ TransactionalEntryLogCompactor.COMPACTED_SUFFIX);
1106+
File finalLogFile = new File(curDir, expectedFinalLogFileName);
1107+
assertTrue(compactedFile.exists(), "Compacted recovery marker should exist");
1108+
assertFalse(finalLogFile.exists(), "Final log file should not exist before makeAvailable");
1109+
1110+
// makeAvailable() is the point where the final entry log file is created
1111+
// on disk. Normal compaction must create "1f4.log", not "500.log".
1112+
compactionLog.makeAvailable();
1113+
assertTrue(finalLogFile.exists(), "Normal compaction path should keep the hex-named final log file");
1114+
assertFalse(new File(curDir, "500.log").exists(),
1115+
"Normal compaction path should not create a decimal-named log file");
1116+
1117+
assertCompactionLogContainsEntry(compactionLog, ledgerId, entryId);
1118+
} finally {
1119+
ReferenceCountUtil.release(entry);
1120+
}
1121+
}
1122+
1123+
private void verifyRecoveredCompactionLogUsesExpectedFinalLogFileName(long previousLogId,
1124+
String expectedFinalLogFileName,
1125+
String wrongDecimalLogFileName)
1126+
throws Exception {
1127+
long sourceLogId = 1L;
1128+
long expectedCompactionLogId = previousLogId + 1;
1129+
long ledgerId = 1234L;
1130+
long entryId = 5L;
1131+
ByteBuf entry = generateEntry(ledgerId, entryId);
1132+
1133+
// Force the next compaction log id to a known value. This lets the test
1134+
// control both the compacted marker name and the expected final log name.
1135+
resetEntryLoggerWithLastLogId(previousLogId);
1136+
1137+
CompactionEntryLog compactionLog = entryLogger.newCompactionLog(sourceLogId);
1138+
assertEquals(expectedCompactionLogId, compactionLog.getDstLogId());
1139+
1140+
try {
1141+
compactionLog.addEntry(ledgerId, entry);
1142+
compactionLog.flush();
1143+
// Simulate a restart after the compaction log was marked compacted but
1144+
// before makeAvailable()/index recovery finished. The remaining file is
1145+
// named "<dst-hex>.log.<src-hex>.compacted".
1146+
compactionLog.markCompacted();
1147+
1148+
File compactedFile = new File(curDir, expectedFinalLogFileName + "."
1149+
+ DefaultEntryLogger.logId2HexString(sourceLogId)
1150+
+ TransactionalEntryLogCompactor.COMPACTED_SUFFIX);
1151+
assertTrue(compactedFile.exists(), "Compacted recovery marker should exist");
1152+
assertFalse(new File(curDir, expectedFinalLogFileName).exists(),
1153+
"Final log file should not exist before recovery");
1154+
1155+
// incompleteCompactionLogs() rebuilds EntryLoggerCompactionEntryLog from
1156+
// the marker. The bug was here: the rebuilt finalLogFile used the decimal
1157+
// compactionLogId instead of the original hex entry log file name.
1158+
Collection<CompactionEntryLog> incompleteLogs = entryLogger.incompleteCompactionLogs();
1159+
assertEquals(1, incompleteLogs.size());
1160+
1161+
CompactionEntryLog recoveredLog = incompleteLogs.iterator().next();
1162+
assertEquals(expectedCompactionLogId, recoveredLog.getDstLogId());
1163+
assertEquals(sourceLogId, recoveredLog.getSrcLogId());
1164+
1165+
// Recovery creates the final entry log when makeAvailable() is called.
1166+
// It must create the hex name and must not leave a decimal-named log.
1167+
recoveredLog.makeAvailable();
1168+
assertTrue(new File(curDir, expectedFinalLogFileName).exists(),
1169+
"Recovery should make the hex-named log file available");
1170+
if (wrongDecimalLogFileName != null) {
1171+
assertFalse(new File(curDir, wrongDecimalLogFileName).exists(),
1172+
"Recovery should not create a decimal-named log file");
1173+
}
1174+
1175+
assertCompactionLogContainsEntry(recoveredLog, ledgerId, entryId);
1176+
} finally {
1177+
ReferenceCountUtil.release(entry);
1178+
}
1179+
}
1180+
1181+
private void assertCompactionLogContainsEntry(CompactionEntryLog compactionLog,
1182+
long ledgerId,
1183+
long entryId) throws Exception {
1184+
AtomicBoolean foundEntry = new AtomicBoolean(false);
1185+
// Scanning uses the compaction log id to resolve the final log file. This
1186+
// catches cases where makeAvailable() created a file under the wrong name.
1187+
compactionLog.scan(new EntryLogScanner() {
1188+
@Override
1189+
public boolean accept(long ledgerId) {
1190+
return true;
1191+
}
1192+
1193+
@Override
1194+
public void process(long ledgerIdFromScanner, long offset, ByteBuf recoveredEntry) {
1195+
assertEquals(ledgerId, ledgerIdFromScanner);
1196+
assertEquals(ledgerId, recoveredEntry.getLong(recoveredEntry.readerIndex()));
1197+
assertEquals(entryId, recoveredEntry.getLong(recoveredEntry.readerIndex() + Long.BYTES));
1198+
foundEntry.set(true);
1199+
}
1200+
});
1201+
assertTrue(foundEntry.get(), "Compacted log should be readable through the compaction log id");
1202+
}
1203+
1204+
private void resetEntryLoggerWithLastLogId(long lastLogId) throws Exception {
1205+
entryLogger.getEntryLoggerAllocator().setLastLogId(curDir, lastLogId);
1206+
entryLogger.close();
1207+
entryLogger = new DefaultEntryLogger(conf, dirsMgr);
1208+
}
1209+
9861210

9871211
/*
9881212
* tests basic logic of EntryLogManager interface for

0 commit comments

Comments
 (0)