Rangsh commented on issue #12441:
URL: https://github.com/apache/seatunnel/issues/12441#issuecomment-5777719148
@Vivek1106-04 @DanielLeens Here is my proposed solution. I would like to go
over it with you and check whether there are any missing parts or unreasonable
aspects.
## 1. Background
Before creating a savepoint checkpoint,
`CheckpointCoordinator.startSavepoint()` waits until any in-flight checkpoint
has drained (`pendingCounter == 0`) or the coordinator has shut down. On
current `dev`, that wait is a sleep-poll **inside** `synchronized (lock)`:
```java
synchronized (lock) {
while (pendingCounter.get() > 0 && !shutdown) {
Thread.sleep(500);
}
// create + start SAVEPOINT_TYPE pending checkpoint
}
```
The periodic trigger path (`tryTriggerPendingCheckpoint`) also enters the
same `lock` before it can skip, create a checkpoint, or re-arm the next timer.
A trigger that fires during this window therefore blocks for as long as the
in-flight checkpoint takes (often seconds under backpressure), instead of
failing fast or re-arming.
`CheckpointManager.triggerSavePoints()` fans out over every pipeline
coordinator with `parallelStream`, so one job suspend can put all of that job's
pipelines into this wait at the same time.
Today the blocked trigger mainly occupies that pipeline's own small
scheduled pool, so the damage is mostly self-contained. With the member-wide
bounded dispatch pool from #12165 (STIP-40: #12339), the same lock-holding can
occupy shared dispatch threads that unrelated pipelines need for their own
triggers and timeout watchdogs. The issue reports the lock-holding defect
(source-verified); full simultaneous occupancy of the dispatch pool is not
required to justify fixing the lock scope.
## 2. Goal
Keep savepoint's "wait for in-flight checkpoint, then create SAVEPOINT_TYPE"
behavior, but **do not hold the coordinator lock while sleeping**. Concurrent
work that needs the same lock (especially periodic triggers) must not be
serialized behind that drain. Shutdown / already-completed / existing in-flight
savepoint handling must stay correct. Checkpoint and savepoint completeness /
recovery semantics must not change.
## 3. Design
### 3.1 Change the wait boundary in `startSavepoint()`
1. Perform the `pendingCounter` drain wait **outside** `synchronized (lock)`
(same 500 ms poll cadence, or equivalent), exiting when `pendingCounter == 0`
or `shutdown`.
2. Enter `synchronized (lock)` only for a short critical section that:
- re-checks shutdown / completed (fail closed with the existing close
reasons if the coordinator ended during the unlocked wait);
- re-checks whether a savepoint is already in flight;
- then `createPendingCheckpoint(..., SAVEPOINT_TYPE)` +
`startTriggerPendingCheckpoint(...)`.
3. Leave `CheckpointManager.triggerSavePoints()` fan-out and savepoint
completion future wiring unchanged in this slice.
This removes the long critical section at its source, so every caller of
that lock benefits on current `dev` and under the shared dispatch pool—without
relying on only the trigger path becoming non-blocking while savepoint still
sleeps under the lock.
### 3.2 Correctness constraints for the unlocked window
While waiting outside the lock, another thread may complete the pending
checkpoint, shut the coordinator down, or start competing coordinator work. The
post-wait locked section must therefore treat the unlocked drain as **stale
until revalidated**, and must not create a savepoint against a terminal
coordinator. Inclusive behavior for "already have an in-flight savepoint"
should remain as today's early return of that savepoint's future.
### 3.3 Tests
Add deterministic unit coverage in `CheckpointCoordinatorTest` (no
full-cluster requirement for the first PR):
1. Force `pendingCounter > 0`, start `startSavepoint()` on one thread so it
enters the drain wait.
2. Concurrently invoke `tryTriggerPendingCheckpoint` (or another path that
needs the same `lock`) and assert it is **not** blocked for the full drain
duration—i.e. sleep-poll no longer runs under the lock.
3. After the pending checkpoint drains, assert the savepoint is still
created successfully.
4. Assert shutdown / already-completed during the unlocked wait still fails
closed with the existing close reasons.
I am not treating "fill the entire shared dispatch pool" as a merge gate for
the first PR; scheduling-delay measurement can stay with #12418. The first PR
should prove the lock-scope fix on current `dev` with the unit tests above.
Please let me know if this direction looks right, or if anything above
should be adjusted before I open the PR. Thanks!
--
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]
