jacklong319 commented on PR #10133: URL: https://github.com/apache/paimon/pull/10133#issuecomment-5844505857
> Reviewed latest head `da05812` for production. The linked DV lag issue gives this feature clear end-to-end value; the latest commit addresses the previously reported per-bucket timer retention and adds worker-churn/shared-pool tests. Local JDK 8 verification passed: four focused classes (11 tests), existing compaction/write classes (26 tests), and normal `paimon-api,paimon-core` compile with Spotless/Checkstyle. > > Two merge blockers remain: > > 1. **P1 — generated configuration docs are missing.** Both Core CI jobs fail in `ConfigOptionsDocsCompletenessITCase`: `Option compaction.task-threads in class org.apache.paimon.CoreOptions is not documented.` The prose docs were updated, but `docs/generated/core_configuration.html` was not regenerated. Please regenerate it per `paimon-docs/README.md` and rerun CI. `git diff --check` also reports trailing whitespace in `docs/docs/primary-key-table/table-mode.md:109`. > 2. **P1 — timer retirement can race with the next reporter on a shared worker.** In `ReporterImpl.getCompactTimer()`, `compactTimers.computeIfAbsent(threadId, ...)` happens before `acquireCompactTimer(threadId)`, while `unregister()` can remove the timer in between. A concrete interleaving: bucket B obtains bucket A's worker timer, bucket A unregisters and removes the last reference, then B acquires the reference and calls `start()` on the now-detached timer. `CompactTask.stopTimer()` calls `getCompactTimer()` again, which creates a fresh timer and calls `finish()` without a matching `start()`; `CompactTimer.finish()` throws `IllegalArgumentException`. This is reachable with the fixed pool/default shared worker when an idle bucket writer is retired while another bucket starts compaction. Make timer lookup/acquisition/removal atomic for a thread ID, and add a concurrent retirement/start regression test. > > The option is worth keeping open, but these issues need fixing before production merge. Thank you, @JingsongLi, for affirming the value of this PR—it is encouraging to see that recognition from the community. I pushed another update to address the two P1 items on the latest head: 1. **Generated configuration docs** — Regenerated `docs/generated/core_configuration.html` so `compaction.task-threads` is documented for `ConfigOptionsDocsCompletenessITCase`. Also removed trailing whitespace in `docs/docs/primary-key-table/table-mode.md` (`git diff --check`). 2. **CompactTimer race on shared workers** — `ReporterImpl.getCompactTimer()` and timer retirement now run under the same per-`threadId` lock, so lookup, ref acquire, and removal are atomic. Reporters still release their compaction-thread timers on `unregister()` with ref-counting when multiple buckets share one worker thread. Added regression tests in `CompactionMetricsTest` for PER_BUCKET churn, shared-thread ref retention, and shared-worker handoff after `unregister`. Could you please take another look when CI is green? -- 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]
