jacklong319 commented on PR #10133: URL: https://github.com/apache/paimon/pull/10133#issuecomment-5811145235
> Reviewed head `072f895` for production use. The end-to-end value is clear: allowing buckets within one long-lived Flink write subtask to compact concurrently can reduce the MOW/DV visibility lag described in #10132. The update addresses the previous feedback on invalid values, documentation, focused executor tests, and Spotless. Local JDK 8 verification passed: the three new test classes 10/10; existing `MergeTreeCompactManagerTest`, `KeyValueFileStoreWriteTest`, and `BucketedAppendFileStoreWriteTest` 26/26; and ordinary `mvn -pl paimon-api,paimon-core -DskipTests compile` including Spotless. Current full CI is still running. > > **P1 — retire compaction timers when PER_BUCKET workers are retired.** `AbstractFileStoreWrite.releaseCompactionExecutor` shuts down and removes each bucket executor, but `CompactionMetrics.ReporterImpl.getCompactTimer()` caches a `CompactTimer` by thread ID in `compactTimers` and never removes that entry; `ReporterImpl.unregister()` removes only the reporter. In a long-lived Flink subtask with changing active partitions/buckets, each replacement worker has a new thread ID. The `compactionThreadBusy` gauge scans every historical timer, so both retained state and scrape cost grow with total buckets ever compacted, even after their writers are gone. I reproduced this with 32 sequential single-thread workers: after shutting each down and unregistering its reporter, the timer map still held all 32 entries (expected 0). The temporary probe was removed from the review checkout. Please bound or retire these timers on worker shutdown and add a churn regression test for `PER_BUCKET`; t he current routing tests only check executor removal. > > This is a feature-specific blocker for the new `-1` mode. The default single-thread path and fixed-size pool do not create the same unbounded worker churn. Thanks @JingsongLi — agreed on the P1 for PER_BUCKET timer retention. I’ll fix timer retirement on worker shutdown, add the churn regression test, and update the PR soon. -- 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]
