Conversation
jt2594838
left a comment
jt2594838
left a comment
There was a problem hiding this comment.
Implementation rationale for the changed recovery paths and regression tests. Validation: 29 targeted tests passed; full-reactor limitation is documented in the PR description.
| return WALMetaData.readFromWALFile( | ||
| file, FileChannel.open(file.toPath(), StandardOpenOption.READ)) | ||
| .getMemTablesId(); | ||
| try (FileChannel channel = FileChannel.open(file.toPath(), StandardOpenOption.READ)) { |
There was a problem hiding this comment.
Close the metadata channel on success and failure so repeated memTable lookups do not leak file handles.
| DataNodeExceptionMetrics.getInstance().recordSuspiciousDiskException(e); | ||
| } | ||
| return Collections.emptySet(); | ||
| // An unreadable WAL may still contain memTables. Treat the ids as unknown so callers |
There was a problem hiding this comment.
Return unknown IDs on a failed read instead of an empty set. This keeps cleanup from treating unreadable data as an empty WAL; the unused Collections import is removed with this change.
| WALInputStream walInputStream = new WALInputStream(logFile); | ||
| // A snapshot supplies the entry boundary for active files and recovered prefixes, whose footer | ||
| // may be absent or damaged. | ||
| WALInputStream walInputStream = new WALInputStream(logFile, true); |
There was a problem hiding this comment.
A supplied metadata snapshot defines the readable entry boundary. Ignore an absent or damaged footer so active-file snapshots and recovered prefixes can be read.
| * that header-only file in place when it is closed. Such a file has no metadata trailer to read, | ||
| * but it is still a valid empty WAL file. | ||
| */ | ||
| public static boolean isEmptyOrHeaderOnly(FileChannel channel) throws IOException { |
There was a problem hiding this comment.
Distinguish valid zero-byte and V2/V3 header-only files from corrupt nonempty files. These valid empty cases have no metadata trailer and must not be quarantined; covered across versions.
| this(logFile, false); | ||
| } | ||
|
|
||
| WALInputStream(File logFile, boolean ignoreMetadata) throws IOException { |
There was a problem hiding this comment.
Recovery scans start after the version header and use the file boundary rather than trusting a corrupt footer length. The normal constructor retains footer-based reading.
| Assert.assertFalse(reader.hasNext()); | ||
| Assert.assertEquals(firstSearchIndex, reader.getFirstSearchIndex()); | ||
| } | ||
| Assert.assertFalse(new WALRepairWriter(logFile).repair(walMetaData)); |
There was a problem hiding this comment.
The one-byte invalid file must be retained as quarantine rather than rewritten into an empty WAL. Assert both the false repair result and preserved original byte.
| } | ||
|
|
||
| @Test | ||
| public void testUnrecoverableFileIsQuarantined() throws IOException { |
There was a problem hiding this comment.
Cover a nonempty invalid file without valid magic and verify repair removes it from the .wal path while keeping the quarantined file.
| } | ||
|
|
||
| @Test | ||
| public void testCorruptedMetadataIsRebuilt() throws IOException, IllegalPathException { |
There was a problem hiding this comment.
Corrupt the metadata entry count while leaving tail magic intact. This catches the previous magic-only completeness check and verifies the rebuilt footer is readable.
| } | ||
|
|
||
| @Test | ||
| public void testFailedRepairPreservesOriginalFile() throws Exception { |
There was a problem hiding this comment.
Supply a stale snapshot that requests an extra entry beyond EOF. Verify the failed rewrite preserves every original byte and leaves no repair temporary file.
| } | ||
|
|
||
| @Test | ||
| public void testStartupCleanupRetainsQuarantinedFile() throws Exception { |
There was a problem hiding this comment.
Verify startup cleanup removes normal WAL/checkpoint files but preserves numbered quarantine bytes, then removes the directory once quarantine is gone.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #18693 +/- ##
============================================
+ Coverage 42.95% 44.46% +1.50%
- Complexity 486 712 +226
============================================
Files 5469 5484 +15
Lines 396623 394381 -2242
Branches 52000 51221 -779
============================================
+ Hits 170384 175362 +4978
+ Misses 226239 219019 -7220 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Caideyipi
left a comment
Caideyipi
left a comment
There was a problem hiding this comment.
I found two recovery-safety issues that should be addressed before merge.
| for (int i = 0; i < entriesNum; ++i) { | ||
| buffersSize.add(buffer.getInt()); | ||
| int size = buffer.getInt(); | ||
| if (size <= 0) { |
There was a problem hiding this comment.
Positive entry sizes still need to be validated against the actual logical WAL data.
A corrupted footer can change an entry size to another positive value, so this check accepts the footer and WALRepairWriter.hasReadableMetadata() reports success without repairing it. I reproduced this with a one-entry V3 WAL by decreasing the stored size by one byte: isRecoveredFromEntries() remained false, WALEntry.deserialize(reader.next()) threw EOFException, and repair() still returned true. During startup, recoverTsFiles() catches that failure and skips the WAL, so an otherwise readable entry is not replayed. A large positive value can also reach ByteBuffer.allocate(size).
Please cross-check the sizes, using overflow-safe arithmetic, against the logical segment lengths and the end marker before treating the footer as valid.
There was a problem hiding this comment.
The full entry-size scan has been reverted because it adds a full logical WAL read/decompression pass to metadata lookup. The footer path remains unchanged in bb74080. This positive-size corruption case remains open; a cheaper validation approach is needed.
There was a problem hiding this comment.
Currently, we only perform a best-effort recovery without affecting the normal path.
| segmentInfo.uncompressedSize = segmentInfo.dataInDiskSize; | ||
| } | ||
| if (segmentInfo.dataInDiskSize <= 0 | ||
| || segmentInfo.uncompressedSize <= 0 |
There was a problem hiding this comment.
uncompressedSize also needs a maximum bound before it is used by ByteBuffer.allocateDirect below. In recovery mode the bytes being inspected are potentially corrupt, and the current predicate only checks that this value is positive.
I reproduced an OutOfMemoryError with a 16-byte WAL containing an LZ4 segment header with dataInDiskSize = 1 and uncompressedSize = 64 MiB under an 8 MiB direct-memory limit. Because OutOfMemoryError is not caught by the surrounding catch (Exception) or by WALReader, the file is not quarantined and startup recovery can abort or hang rather than continue.
Please reject values above the maximum logical segment size the writer can produce, with overflow-safe checks, before allocating the direct buffer.
There was a problem hiding this comment.
Addressed in bb74080. Recovery now rejects a declared logical segment size above WALBuffer.ONE_THIRD_WAL_BUFFER_SIZE before allocating direct buffers, and bounds compressed payloads using the compressor maximum for that capacity. The 16-byte LZ4 regression includes the declared payload and expects EOFException from the pre-allocation check. All 14 WALFileTest tests pass, with Checkstyle and Spotless.
| } | ||
| // Recovery inspects untrusted headers. Bound allocations by the writer's configured segment | ||
| // capacity so a tiny corrupt payload cannot request a huge decompression buffer. | ||
| if (recoveringEntries |
There was a problem hiding this comment.
This guard follows the WAL writer segment capacity and runs before either direct buffer allocation. It prevents a corrupt recovery header from turning a tiny on-disk payload into an unbounded decompression allocation; the oversized LZ4 regression passed as EOFException before decompression.
|
|
||
| try (WALInputStream input = new WALInputStream(walFile, true)) { | ||
| // A decompressor failure is wrapped in IOException; this must fail before reaching it. | ||
| assertThrows(EOFException.class, input::read); |
There was a problem hiding this comment.
The test includes the complete one-byte compressed payload, so a truncated physical read cannot make it pass. EOFException specifically verifies rejection by the size guard rather than the IOException wrapper used for decompressor failures. All 14 WALFileTest tests passed.
| // Recovery inspects untrusted headers. Bound allocations by the writer's configured segment | ||
| // capacity so a tiny corrupt payload cannot request a huge decompression buffer. | ||
| if (recoveringEntries | ||
| && (segmentInfo.uncompressedSize > WALBuffer.ONE_THIRD_WAL_BUFFER_SIZE |
There was a problem hiding this comment.
This limit is derived from the current wal_buffer_size, but a WAL may have been written with a larger value before a configuration change. On restart, recovery will reject a valid segment from that file. If it is the first segment and the footer is damaged, the scan finds no entries and quarantines the WAL, losing its readable data. Could the recovery bound be independent of the current writer buffer setting (while still limiting allocations)?
| for (int i = 0; i < entriesNum; ++i) { | ||
| buffersSize.add(buffer.getInt()); | ||
| int size = buffer.getInt(); | ||
| if (size <= 0) { |
There was a problem hiding this comment.
Checking only size > 0 still accepts a corrupted entry length such as Integer.MAX_VALUE in an otherwise parseable footer. readFromWALFileWithoutRecovery then treats the footer as valid, and WALByteBufReader.next() calls ByteBuffer.allocate(size) before reading any entry bytes, potentially exhausting the process heap. Please validate entry sizes against the actual readable data (or impose a safe allocation bound) so this case enters recovery instead.
3 participants
Description
When a WAL footer is missing or corrupted, metadata readers currently fail even if complete entries remain readable. Recover metadata from the readable entry prefix, recognize empty/header-only WAL files, and quarantine nonempty files with no recoverable entries as
.broken(with a numbered suffix on collision).V3 writer progress stored only in a damaged footer cannot be reconstructed from entry bodies; recovery uses the existing unknown/default progress values. Incomplete compressed segments cannot be decoded, so recovery retains complete readable entries before them.
Validation
WALFileTest,WALRepairWriterTest,WALMetaDataV3CompatibilityTest, andProgressWALReaderTest, using freshly compiled affected sources and existing local dependency artifacts in an isolated output directory.git diff --checkpassed.This PR has:
Key changed classes: WALMetaData, WALInputStream, WALReader, WALByteBufReader, WALFileVersion, WALWriter, WALBuffer, WALNode, WALNodeRecoverTask, WALRepairWriter.