jacklong319 commented on PR #10133: URL: https://github.com/apache/paimon/pull/10133#issuecomment-5976899848
> Reviewed head `0107e685` for production use. Requirement fit: SUPPORTED; implementation: FINDINGS. > > The cross-bucket capacity knob addresses a credible production bottleneck. The default still uses one shared compaction thread, and per-bucket task serialization remains in place. However, the new parallel modes expose a shared-reader data corruption path for bucketed append tables, and timer retirement changes the busy metric even with the default configuration; see the inline comments. > > Verification: normal JDK 8 build and 47 focused/adjacent tests passed, including formatting checks. Additional deterministic probes reproduced both findings. The append probe used actual Parquet input and output files and the configured executors; a latch controlled the interleaving around the existing cached caster without changing the returned values. One thread preserved both rows; two threads and per-bucket executors persisted an incorrect nested value. > > A smaller initial design would support only positive thread counts: keep the existing executor for 1 and use one bounded shared pool for N > 1. This removes the per-bucket executor map, key, release/recreation lifecycle, mode enum, and the need to retire timers for churned workers. Keep the shared-counter synchronization. This simplification still needs isolated append reader/cast state before enabling append concurrency; alternatively, initially restrict the feature to the validated primary-key compaction path. I would resolve the P1 before deploying either parallel mode. @JingsongLi Thanks for the detailed review on the parallel compaction findings. I pushed a follow-up that addresses the P1/P2 items while keeping `compaction.task-threads=-1` (PER_BUCKET). **[P1] Append compaction reader / cast state** - Root cause: `BaseAppendFileStoreWrite` shared a single `RawFileSplitRead` whose cached `FormatReaderMapping` (including nested cast state) was reused across parallel compaction workers. - Fix: for `FIXED_POOL` and `PER_BUCKET`, each compaction worker thread uses its own `RawFileSplitRead` copy with a fresh mapping cache (`copyWithFreshReaderMappings`). `SINGLE` keeps the previous shared reader (no extra overhead). - Regression: `ParallelAppendCompactionReaderIsolationTest` — two buckets run `compactRewrite` in parallel after nested schema evolution (`payload.val` INT → BIGINT), with `task-threads=2` and `-1`; each bucket’s marker values stay isolated. **[P2] `compactionThreadBusy` / CompactTimer lifetime** - Fix: retire `CompactTimer` on reporter `unregister()` **only** when the write uses `PER_BUCKET` (dedicated per-bucket workers). For `SINGLE` and `FIXED_POOL`, timers are **not** removed on bucket reporter unregister, so the 60s busy window is preserved for long-lived shared workers. - Removed ref-count bookkeeping on the default path; updated `CompactionMetricsTest` accordingly. PER_BUCKET executor routing and earlier counter synchronization (`sharedCounterLock`) are unchanged. Could you take another look when CI is green? Happy to follow up separately if you see gaps on PK merge-tree paths or external compact executor combinations. -- 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]
