shoemoney commented on PR #8573: URL: https://github.com/apache/hbase/pull/8573#issuecomment-5616711158
The simplified approach from the 2026-08-26 review (always return `[MIN_VALUE, cutOffTimestamp]` without traversing `filesToCompact` for `CUSTOM_TIERING_TIME_RANGE`) is implemented in `CustomDateTieredCompactionPolicy#getCompactBoundariesForMajor`. That landed in 7637d74e55, and `filesToCompact` is no longer referenced anywhere in the method body. On the 5 tests flagged in my 2026-09-01 comment: I went with updating the tests to match the new intended behavior rather than a narrower fix, landed in 681823bf11. - `TestCustomCellTieredCompactionPolicy#testGetCompactBoundariesForMajorNoOld/OneOnEachSide/OneCrossing`: `getCompactBoundariesForMajor` now unconditionally returns a 2-element list regardless of input, so any expected size other than 2 was mechanically wrong once the traversal was removed. Changed the three asserts from 1/3/3 to 2/2/2. - `TestCustomCellTieredCompactor#testCustomCellTieredCompactor` and `#testCustomCellTieredCompactorWithRowKeyDateTieringValue`, first-compaction blocks: the comment claiming the first compaction "would have no means to detect more than one tier" and its `assertEquals(1, numHFiles)` checks are no longer true, since the boundaries no longer depend on file metadata. Changed to `assertEquals(2, ...)` (both tables in the RowKeyDateTieringValue variant) and removed the now-inaccurate comment. - Same two methods also had exact-value TimeRangeTracker asserts in the first-compaction block (`assertEquals(recordTime - 11 years, min)`, `assertEquals(recordTime, max)`). With the split now producing 2 tier-homogeneous files (one old timestamp, one recent), a correctly split file can only contain one of the two literal timestamps the test writes, so I changed those to `assertEquals(min, max)` per file. That is not a check I loosened to dodge anything: it is the same pattern the original author already used in these two methods' second-compaction blocks, which already produced 2 files before this PR touched anything. I extended that existing convention to the first-compaction block now that it also produces 2 files, rather than inventing a new, weaker standard. Caveat worth flagging: min == max confirms per-file homogeneity but not that the surviving value is specifically the old or recent timestamp rather than some other value, same limitation the original second-compaction ass erts already had. On "No need to declare an array here" on `CustomDateTieredCompactionPolicy.java`: the `boolean[] hasMissing` it flagged was part of the `MutableLong`/`forEach` traversal block, which the simplification in 7637d74e55 removed wholesale rather than editing in place. The file no longer has `hasMissing`; the only remaining array in it is the unrelated `byte[] timeRangeBytes` local in `shouldPerformMajorCompaction`. -- 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]
