Conversation
…me range The timestamp in a WAL's name is its creation time, which only bounds its entries from below. A WAL stays open until it rolls, so one created before startTime can still hold entries in range, and comparing the name against startTime threw the whole file away. Compare against the modification time instead, which bounds the entries from above, but only once the file is closed: HDFS leaves mtime at the creation time through hflush and hsync. Gate it on DistributedFileSystem.isFileClosed and keep anything we cannot answer for, including non-HDFS filesystems. The call is short-circuited. wal.start.time defaults to Long.MIN_VALUE, so a job that does not ask for a start time never reaches it, and one that does pays it only for files mtime alone would prune. TestWALRecordReader.testPartialRead asserted the old behaviour: it writes an entry at exactly startTime into a WAL created earlier and expected that file to be skipped. Corrected to expect both splits, asserting the entry that was being lost.
There was a problem hiding this comment.
🟢 Approval recommended
The change is localized, matches the described correctness fix for WAL spanning behavior, and is backed by updated and newly added test coverage targeting the new selection logic.
Pull request overview
This PR fixes WAL time-range filtering in hbase-mapreduce by preventing WALInputFormat from incorrectly skipping WAL files whose creation timestamp predates startTime but that remained open long enough to contain in-range entries. It does so by only pruning on the startTime boundary when the WAL is confirmed closed and its final modification time is before startTime.
Changes:
- Update
WALInputFormatfile selection to (a) still skip WALs created afterendTime, but (b) avoid skipping WALs created beforestartTimeunless the WAL is known closed and hasmtime < startTime. - Add HDFS-aware “is file closed” detection (including unwrapping
HFileSystem) to makemtimemeaningful only for closed files. - Extend/adjust unit tests to cover WALs spanning the
startTimeboundary and the closed-vs-openmtimebehavior.
File summaries
| File | Description |
|---|---|
| hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/WALInputFormat.java | Switches start-time pruning from WAL-name timestamp to (closed-only) modification-time checks; introduces isClosed using DistributedFileSystem.isFileClosed. |
| hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALInputFormat.java | Updates existing addFile test expectations and adds targeted coverage for “use mtime only when closed” behavior. |
| hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALRecordReader.java | Updates partial-read expectations to ensure WALs spanning the startTime boundary are retained and read correctly. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Yuta Imazu <yuta.imazu@gmail.com>
| // The modification time is the upper bound, but HDFS leaves it at the creation time until | ||
| // the file is closed, so it is only meaningful once the file is. Order the checks so the | ||
| // extra RPC is only paid for files that the modification time alone would prune. | ||
| if (timestamp < startTime && lfs.getModificationTime() < startTime && isClosed(fs, lfs.getPath())) { |
There was a problem hiding this comment.
Sorry for meat-proxying, but Codex says
[P1] Refresh file status after confirming that the WAL is closed — hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/WALInputFormat.java:372-372
If a WAL closes afterlistLocatedStatuscaptures its status but beforeisClosedruns,lfsstill contains the open file's creation-time mtime. This check then sees a closed file and incorrectly prunes it despite in-range entries and a newer actual mtime, silently omitting edits from WALPlayer. Re-fetch the file status after confirming closure before using its mtime to prune, retaining the file if that refresh fails.
With the timestamp < startTime guard, this patch still is an improvement, though.
There was a problem hiding this comment.
Ah, yeah, that's possible. Pretty unlikely in real-world use cases though, but it's better to be right.
There was a problem hiding this comment.
Addressed in 5adb255. Could you take another look?
listLocatedStatus captures mtime while the file is open (mtime = creation time). If the file closes before isFileClosed runs, the stale mtime can cause incorrect pruning. Re-fetch FileStatus after confirming closure to get the final mtime. - Rename isClosed to isClosedBefore, fold mtime check and re-fetch - Stale mtime check remains as fast path to skip the RPC
ViewDistributedFileSystem passes the instanceof DistributedFileSystem check but throws UnsupportedOperationException when the mounted filesystem does not support isFileClosed(). Treat this the same as a non-HDFS filesystem: keep the file.
thirdTs is sampled immediately after WAL shutdown, so it can equal the WAL's final modification time. Using it directly as startTime makes isClosedBefore retain the WAL, failing the assertTrue(splits.isEmpty()) assertion. Offset by 1 ms to make the boundary unambiguous.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
There was a problem hiding this comment.
🟡 Changes recommended
Two unresolved Javadoc updates remain in WALInputFormat.java.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
hbase-mapreduce/src/main/java/org/apache/hadoop/hbase/mapreduce/WALInputFormat.java:423
- The
startTimeJavadoc immediately above still says that a timestamped file is filtered out whenever its filename timestamp is older thanstartTime, but this method now intentionally retains such files when they are open or when their closed-file modification time reaches the window. Please update that parameter documentation so it describes the creation-time lower bound and the closed-file mtime check; otherwise the method contract is misleading for maintainers and callers.
static void addFile(List<FileStatus> result, FileSystem fs, LocatedFileStatus lfs, long startTime,
- Files reviewed: 3/3 changed files
- Comments generated: 1
- Review effort level: Lite
The old description implied pure name-based filtering, but files created before startTime are only dropped when confirmed closed.
Jira: HBASE-30377
The timestamp in a WAL's name is its creation time, which only bounds its entries from below. A WAL stays open until it rolls, so one created before
startTimecan still hold entries in range, and comparing the name againststartTimethrew the whole file away.Fix
Compare against the modification time instead, which bounds the entries from above. That is only meaningful once the file is closed, since HDFS leaves mtime at the creation time through
hflushandhsync, so it is gated onDistributedFileSystem.isFileClosed. Anything we cannot answer for, including non-HDFS filesystems, is treated as open and kept.End-to-end test
I verified the fix on a 6-node test cluster running on a local Kubernetes cluster. Without the 1-hour
logroll.periodworkaround, WALPlayer correctly processes all WAL files.This is safe and conservative
With @mosmeh's suggestion applied, this patch never excludes files that the previous code included.