Copilot commented on code in PR #8646:
URL: https://github.com/apache/hbase/pull/8646#discussion_r4015540899
##########
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 assumes a stable ordering of `splits` (i.e., that the WAL spanning
`startTime` is always `splits.get(0)`). `InputFormat#getSplits()` ordering can
vary depending on filesystem listing order, making this potentially flaky.
Consider asserting the set of split contents regardless of order (e.g., run
`testSplit` against both splits and verify both expected keys are present), or
deterministically sort splits by path before indexing.
##########
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 updated comment refers to the last WAL's *modification time*, but the
variable name `thirdTs` reads like a timestamp derived from the WAL
name/creation time. If `thirdTs` is not actually the file mtime, the comment is
misleading—either adjust the comment to match what `thirdTs` represents, or
rename/compute the value to reflect modification time.
--
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]
