DanielLeens commented on PR #12165: URL: https://github.com/apache/seatunnel/pull/12165#issuecomment-5775112569
@Vivek1106-04 Thanks for running this - independent confirmation with numbers is exactly the right way to settle Issue 1. Two things this benchmark clarifies: 1. **Design claim**: confirmed and decisive. A flat ~10us scheduling delay from 1 to 500 pipelines against a knee between 100 and 500 on `dev` settles the "is this worth it" question on its own. 2. **The savepoint half of Issue 1 is now verified, not just inferred from source reading.** Your trace of `startSavepoint` holding `lock` across its sleep-poll, with `tryTriggerPendingCheckpoint` taking that same `lock`, is exactly the mechanism I flagged. The "flat to 31% utilization, then four orders of magnitude past saturation" shape is also a good argument for landing the scheduling-delay metric ahead of any pool-size option, as you say. On your question - here is my call, split by the two parts of Issue 1 since they have different costs to fix: - **Watchdog IMap I/O hop** (the other half of Issue 1: the expiry path runs distributed IMap reads/writes with retry on the dispatch thread). This is not a design question, it is the same shape as `31a454f35` - hand the failure handling to `executorService` and keep the dispatch lambda to the cheap lookup. I would still like that in this PR; it is a small change and removes half the coupling outright regardless of how the savepoint question lands. - **Savepoint lock-holding**: given you cannot reach the simultaneous-occupancy case without a narrow interval/window coincidence, and the benchmark shows the win is decisive at scale, I am fine treating "stop sleeping while holding `lock`" as a tracked follow-up rather than a blocker here - that is the larger, separate change I called out as option B in my last review. Please do correct the "no I/O, no RPC" line in the class Javadoc and in STIP-40 to describe the now-verified savepoint-lock case, so the option decision on the STIP thread is made on accurate facts rather than the earlier assumption. So: watchdog hop in this PR, savepoint-lock as a tracked follow-up with corrected docs. With that plus a green CI run on the failed jobs (most of the current red ones reproduce on plain `dev`, per the CI notes in my last review), this is ready to merge from my side. Appreciate the rigor here - registering the benchmark in CI so the numbers can be reproduced independently is a good call too. -- 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]
