KKcorps commented on PR #19115: URL: https://github.com/apache/pinot/pull/19115#issuecomment-5127382949
I traced the same-partition call path before deciding on synchronization. ConsumerCoordinator uses a one-permit semaphore per partition. When the parallel-consumption policy lets the next segment start during build or download, the old segment closes its consumer and releases that permit first, so only one consumer thread reaches MutableSegmentImpl -> updateRecord -> merge at a time. Making merge synchronized would therefore add monitor cost on the normal hot path. It would also be only a partial safety net if that invariant were ever broken: _reusePreviousRow is initialized before merge, and _reusePreviousRow plus _reuseMergeResultHolder are cleared after merge, outside the proposed monitor. Those shared scratch objects could still race. I kept merge unsynchronized and made the confinement contract explicit with @NotThreadSafe. If concurrent same-partition merge calls are supported later, the complete fix should use per-thread/per-consumer handler and scratch state, or lock the entire updateRecord critical section. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
