messere1 commented on PR #11355: URL: https://github.com/apache/rocketmq/pull/11355#issuecomment-6098304917
Thanks for the review, and glad the overall approach checks out. Responses to the two minor observations (addressed in 01e5b7d09): **1. `currentTimeMillis` vs `nanoTime`** — deliberate choice: the tombstone stamp is compared against the same `System.currentTimeMillis()` horizon that the very same sweep already uses for subscription expiry (`cleanupExpiredSubscriptions` compares `LiteSubscription.getUpdateTime()` against a `currentTimeMillis()` read, one code path above `removeExpired`). Mixing `nanoTime` into that comparison would put the two expiries in different clock domains. The TTL window is minutes, so NTP slew is well within tolerance. **2. Timing margin of the sleep-based tests** — good instinct, though note the margin direction is already safe: `Thread.sleep` never undershoots, so a slow runner only ages the tombstone *further past* the TTL (the pass direction for the expiry assertions — the sleeps live in `testExclusiveEviction_TombstoneExpiredForClientThatNeverSyncsAgain` and `ExclusiveEvictionTombstonesTest#testRemoveExpired`; `testExclusiveEviction_FreshTombstoneSurvivesExpirySweep` has no sleep and pits a zero-age tombstone against a 60s TTL, which cannot flake). That said, I bumped both sleeps from 50ms to a 10x margin (200ms vs the 20ms TTL) in 01e5b7d09, which additionally rides out a backwards clock step of up to ~180ms between stamp and sweep, at negligible test-runtime cost. Both test classes re-run green (54/54). Clock injection was considered and rejected for now: it would touch the production registry/tombstone classes for a non-blocking nit, and the repo's lite tests consistently use real-clock aging (e.g. `AbstractLiteLifecycleManagerTest`). -- 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]
