shangxinli commented on PR #19868: URL: https://github.com/apache/hudi/pull/19868#issuecomment-5638190912
Thanks @voonhous, this was a genuinely useful review — I verified all four majors against the code and they all hold. Summary of the revision: **Fixed** - The gauge no longer freezes on the failure path. It is now sampled once in the `finally` block of `performTableServices()`. One refinement on your suggestion: `getActiveTimeline()` there is not enough either, since every caller passes `requiresTimelineRefresh=false` (`HoodieTableMetadataWriter.java:178`) and neither `runAnyPendingCompactions` nor `writeClient.compact` refreshes this meta client — so it samples off `reloadActiveTimeline()`, wrapped so it can neither mask a table service failure nor become one. - Prefixed with the MDT table name so `CloudWatchReporter` no longer drops it. - Reworded the plateau behavior. I did not adopt the "alert at 2x" guidance, because the plateau depends on the archival config as well as the compaction threshold — now that the sample is post-service, a healthy table returns to about zero each cycle, so the condition is "stays at or above the threshold across consecutive cycles". **Scope cut** Dropped `totalBaseFileCount`, `totalLogFileCount` and `logToBaseFileRatioPercent` rather than fixing each. Between your ratio-weighting case, the detailed-metrics default, and the fact that all three are arithmetic over per-partition gauges the same call already emits, they were not earning their place in a compaction-health PR. That resolves five of the comments at once, including the missing `updateSizeMetrics` coverage — though that gap is real and pre-existing, and I am happy to file it separately. **Tests** Folded into one `@ParameterizedTest`, unified onto a single setup helper, reverted the `static`, fixed the config key. Added three cases your review implied: completion-time ordering (`0015`/`0035`), compaction-last, and the failure path. I also added a case that pins the reload — the cached and reloaded timelines are stubbed to different values, so it fails if the sample goes back to `getActiveTimeline()`. Each of the three fixes was mutation-checked: reverting the reload fails 1 test, dropping the name prefix fails 10, and moving the sample out of `finally` fails 5. **Not doing here** - Emitting from the Flink streaming-write compaction pipeline. Real gap, confirmed at both guards — documented on the constant and left as a follow-up rather than widened into this PR. - Wiring up or deleting `STAT_LAST_COMPACTION_TIMESTAMP`. Dead since 2020 and independent of this change. An age-since-compaction gauge is a good idea and I would rather propose it on its own. 30/30 tests pass locally in `hudi-client-common`. -- 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]
