Jackie-Jiang commented on PR #14643: URL: https://github.com/apache/pinot/pull/14643#issuecomment-6007328157
I'm still not convinced this change is necessary based on the evidence provided. A slow ZK refresh can block other callers, but refresh runs when the cache is invalid. Those callers also need a successful refresh before they can obtain a usable leader. With this change, they normally get `null` while refresh is in progress because `_cachedControllerLeaderValid` remains false; they do not reuse the stale leader as the description suggests. Segment-consumed requests then return `NOT_SENT` and retry, so this does not by itself improve leader discovery or let segment completion progress. There is a potential benefit for shutdown responsiveness: a thread waiting to enter a synchronized method cannot interrupt that wait, whereas a caller returning `null` can get back to its stop checks. Could you provide a concrete operational failure and a regression test demonstrating that improvement? The existing methods already serialize the shared state correctly, so I don't see the original race condition being fixed here. The new implementation also allows an invalidation to be lost: a slow refresh reads old leadership, another thread performs an eligible invalidation, and the refresh subsequently sets `_cachedControllerLeaderValid = true`. The invalidation timestamp has already advanced, suppressing another invalidation for up to 30 seconds. Making the timestamp atomic does not coordinate invalidation with refresh publication. If we proceed with this approach, that needs to be addressed, along with rechecking validity after acquiring `tryLock()` to avoid a redundant refresh. The existing tests are sequential. Please add controlled concurrent tests covering a blocked refresh with another lookup/invalidation, and invalidation during refresh. For now, I'd keep the current synchronization and investigate the ZK refresh latency unless we can demonstrate a concrete operational benefit from changing the locking. -- 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]
