Copilot commented on code in PR #8646:
URL: https://github.com/apache/hbase/pull/8646#discussion_r4014834591
##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALRecordReader.java:
##########
@@ -190,9 +190,12 @@ public void testPartialRead() throws Exception {
jobConf.setLong(WALInputFormat.START_TIME_KEY, ts + 1);
jobConf.setLong(WALInputFormat.END_TIME_KEY, ts1 + 1);
splits = input.getSplits(MapreduceTestingShim.createJobContext(jobConf));
- assertEquals(1, splits.size());
+ assertEquals(2, splits.size());
+ // The 1st file was created before startTime but stayed open until it
rolled, so its 2nd
+ // entry, written at exactly startTime, is in-range.
+ testSplit(splits.get(0), Bytes.toBytes("2"));
// Only the 1st entry from the 2nd file is in-range.
- testSplit(splits.get(0), Bytes.toBytes("3"));
+ testSplit(splits.get(1), Bytes.toBytes("3"));
Review Comment:
This test now assumes a deterministic ordering of `splits` (i.e., that the
split containing entry \"2\" is always index 0 and \"3\" is always index 1).
`InputFormat#getSplits` ordering can vary by filesystem iteration order, which
can make this test flaky. Prefer asserting independent of order (e.g., locate
the split by its WAL path/name, or sort splits by path before calling
`testSplit`).
##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALInputFormat.java:
##########
@@ -65,34 +66,98 @@ public static void setupClass() throws Exception {
@Test
public void testAddFile() {
List<FileStatus> lfss = new ArrayList<>();
+ // a plain FileSystem is never reported closed, so nothing is skipped on
the startTime side
+ FileSystem fs = Mockito.mock(FileSystem.class);
LocatedFileStatus lfs = Mockito.mock(LocatedFileStatus.class);
long now = EnvironmentEdgeManager.currentTime();
Mockito.when(lfs.getPath()).thenReturn(new Path("/name." + now));
- WALInputFormat.addFile(lfss, lfs, now, now);
+ WALInputFormat.addFile(lfss, fs, lfs, now, now);
assertEquals(1, lfss.size());
- WALInputFormat.addFile(lfss, lfs, now - 1, now - 1);
+ WALInputFormat.addFile(lfss, fs, lfs, now - 1, now - 1);
assertEquals(1, lfss.size());
- WALInputFormat.addFile(lfss, lfs, now - 2, now - 1);
+ WALInputFormat.addFile(lfss, fs, lfs, now - 2, now - 1);
assertEquals(1, lfss.size());
- WALInputFormat.addFile(lfss, lfs, now - 2, now);
+ WALInputFormat.addFile(lfss, fs, lfs, now - 2, now);
assertEquals(2, lfss.size());
- WALInputFormat.addFile(lfss, lfs, Long.MIN_VALUE, now);
+ WALInputFormat.addFile(lfss, fs, lfs, Long.MIN_VALUE, now);
assertEquals(3, lfss.size());
- WALInputFormat.addFile(lfss, lfs, Long.MIN_VALUE, Long.MAX_VALUE);
+ WALInputFormat.addFile(lfss, fs, lfs, Long.MIN_VALUE, Long.MAX_VALUE);
assertEquals(4, lfss.size());
- WALInputFormat.addFile(lfss, lfs, now, now + 2);
+ WALInputFormat.addFile(lfss, fs, lfs, now, now + 2);
assertEquals(5, lfss.size());
- WALInputFormat.addFile(lfss, lfs, now + 1, now + 2);
- assertEquals(5, lfss.size());
- Mockito.when(lfs.getPath()).thenReturn(new Path("/name"));
- WALInputFormat.addFile(lfss, lfs, Long.MIN_VALUE, Long.MAX_VALUE);
+ // created before startTime, but it may have stayed open and collected
in-range entries
+ WALInputFormat.addFile(lfss, fs, lfs, now + 1, now + 2);
assertEquals(6, lfss.size());
- Mockito.when(lfs.getPath()).thenReturn(new Path("/name.123"));
- WALInputFormat.addFile(lfss, lfs, Long.MIN_VALUE, Long.MAX_VALUE);
+ Mockito.when(lfs.getPath()).thenReturn(new Path("/name"));
+ WALInputFormat.addFile(lfss, fs, lfs, Long.MIN_VALUE, Long.MAX_VALUE);
assertEquals(7, lfss.size());
- Mockito.when(lfs.getPath()).thenReturn(new Path("/name." + now + ".meta"));
- WALInputFormat.addFile(lfss, lfs, now, now);
+ Mockito.when(lfs.getPath()).thenReturn(new Path("/name.123"));
+ WALInputFormat.addFile(lfss, fs, lfs, Long.MIN_VALUE, Long.MAX_VALUE);
assertEquals(8, lfss.size());
+ Mockito.when(lfs.getPath()).thenReturn(new Path("/name." + now + ".meta"));
+ WALInputFormat.addFile(lfss, fs, lfs, now, now);
+ assertEquals(9, lfss.size());
+ }
+
+ private static boolean isKept(FileSystem fs, long created, long mtime, long
start, long end) {
+ return isKept(fs, created, mtime, mtime, start, end);
+ }
+
+ private static boolean isKept(FileSystem fs, long created, long staleMtime,
long refreshedMtime,
+ long start, long end) {
+ List<FileStatus> result = new ArrayList<>();
+ Path path = new Path("/name." + created);
+ LocatedFileStatus lfs = Mockito.mock(LocatedFileStatus.class);
+ Mockito.when(lfs.getPath()).thenReturn(path);
+ Mockito.when(lfs.getModificationTime()).thenReturn(staleMtime);
+ try {
+ FileStatus refreshed = Mockito.mock(FileStatus.class);
+ Mockito.when(refreshed.getModificationTime()).thenReturn(refreshedMtime);
+ Mockito.when(fs.getFileStatus(path)).thenReturn(refreshed);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
Review Comment:
This helper wraps `IOException` in a `RuntimeException` even though the
calling test already declares `throws Exception`. Simplifying this to let the
exception propagate (or moving the stubbing into the calling test method that
already throws) will make failures easier to diagnose and reduce unnecessary
control flow.
##########
hbase-mapreduce/src/test/java/org/apache/hadoop/hbase/mapreduce/TestWALRecordReader.java:
##########
@@ -245,9 +248,9 @@ public void testWALRecordReader() throws Exception {
assertEquals(1, splits.size());
testSplit(splits.get(0), Bytes.toBytes("1"));
- // now set a start time
+ // now set a start time strictly after the last WAL's modification time
jobConf.setLong(WALInputFormat.END_TIME_KEY, Long.MAX_VALUE);
- jobConf.setLong(WALInputFormat.START_TIME_KEY, thirdTs);
+ jobConf.setLong(WALInputFormat.START_TIME_KEY, thirdTs + 1);
Review Comment:
The comment states `START_TIME_KEY` is set strictly after the last WAL's
*modification time*, but the variable name `thirdTs` reads like a timestamp
from the WAL name/creation time. If `thirdTs` is not actually the last WAL
mtime, the comment is misleading and the test may be relying on an incorrect
assumption. Consider either (a) renaming/deriving the value from the actual
file modification time used by the logic under test, or (b) updating the
comment to match what `thirdTs` represents.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
