hbgstc123 commented on PR #9660: URL: https://github.com/apache/paimon/pull/9660#issuecomment-5831005533
> Rechecked current head `7932c9bafd` after the two formatting-only commits (`7c9dceb63d`, `7932c9bafd`). They do not change the metric lifecycle finding in my review: `CompactionFastPathMetrics` is still registered in `withMetricRegistry` and not closed from `BaseAppendFileStoreWrite.close()`. The earlier JDK 8 data and format test results remain applicable; affected CI on this head is pending. Thanks for the detailed review. Both points addressed in `ef566eecf`. **Metric lifecycle** — fixed as suggested: - `CompactionFastPathMetrics` is now closeable: `close()` delegates to `metricGroup.close()`, mirroring the `BlobFetchMetrics` pattern, and `BaseAppendFileStoreWrite.close()` closes it right after `blobFetchMetrics`. - `withMetricRegistry` now only constructs and registers the `compactionFastPath` group when `append.compaction.row-group-copy.enabled=true`. With the default config, no counters are registered at all. Added `CompactionFastPathMetricsTest` (4 tests): - `testMetricRegistration`: group name, `table` variable, and all 14 counters (1 hit + 13 miss reasons). - `testCloseClosesMetricGroup`: `close()` closes the underlying group, using a tracking `MetricGroup`. - `testWriterClosesMetricGroupWhenEnabled`: the probe scenario from your review — normal append writer, `withMetricRegistry`, `close()`, asserting the `compactionFastPath` group is registered and then closed. - `testWriterSkipsMetricGroupWhenDisabled`: feature disabled → no group is registered. **Verification on JDK 8**: `CompactionFastPathMetricsTest` 4/4, `ParquetFastPathCompactRewriterTest` 14/14, `SimpleStatsMergerTest` 6/6, `AppendOnlyTableCompactionTest` 11/11 passed; `spotless:check` and `checkstyle:check` are clean. -- 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]
