wchevreuil commented on PR #8573: URL: https://github.com/apache/hbase/pull/8573#issuecomment-5617911514
> 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 [7637d74](https://github.com/apache/hbase/commit/7637d74e554c86328dc452ac26cb7f12f6098796), 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 [681823b](https://github.com/apache/hbase/commit/681823bf11ab48154040efceeb3540ef8cbf42fe). > > * `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 a sserts 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 [7637d74](https://github.com/apache/hbase/commit/7637d74e554c86328dc452ac26cb7f12f6098796) 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`. Thanks for the work here, and yes, the tests should be updated as you did here to reflect the new behaviour. This looks ready to be merged, but we need to a jira to link it first. Please @me on the jira once you create it so that I can assign it to you and proceed with the PR merge. -- 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]
