Vivek1106-04 commented on PR #12277:
URL: https://github.com/apache/seatunnel/pull/12277#issuecomment-5650665747
Thanks both for the detailed reviews. All five issues from @DanielLeens and
both findings from @goutamadwant are addressed in f6d0251.
**Blockers**
1. *Stale Javadoc on `run()`* - the execution-flow Javadoc now sits on
`runBusWork()`, where that logic lives, and it also documents the new
queue-polling accounting. `run()` has a short comment saying it only wraps
`runBusWork()` with the guaranteed release of what the worker holds.
2. *TOCTOU on the replacement decision* - the check-then-act is gone, and
@goutamadwant's review showed the counter was measuring the wrong thing as
well: `sharedCooperativeWorkers` counted workers that had not been promoted,
including workers blocked inside a task call that cannot poll the queue at all.
Their repro (per job budget 1, latch-blocked tasks, a task queued behind them
never starting) was real. So the fix is both halves:
- `queuePollingWorkers` counts workers actually waiting on the shared
queue, `pendingQueueWorkers` counts workers started but not polling yet, and a
worker leaves both counts while it runs a call.
- `ensureQueueIsServed()` starts a worker only when the sum of the two is
zero, under a lock with a double check, so concurrent denials agree on one
worker instead of each spawning its own.
**Non-blocking**
3. `CooperativeWorkerBudget.tryAcquire` now returns a `PromotionDecision`
(`ADMITTED`, `NODE_BUDGET_EXHAUSTED`, `JOB_BUDGET_EXHAUSTED`), and denials are
logged at WARN with the reason, both current counts against their limits, and
the total denials.
4. Javadoc added to both `getPromotedWorkers` accessors (and the other
accessors while there).
5. Class-level Javadoc added to `CooperativeWorkerBudgetTest`.
**On the flaky `<= 3` assertion**: removed. It encoded the broken
accounting. The service test now asserts per task progress instead, and the new
regression asserts a bound derived from the workload itself (one worker per
blocked call plus the workers keeping the queue served).
**On what the budget promises**: your point that the defaults ship the
protection opt-in is correct, and the docs now also state the limit of the
guarantee: the budget bounds promotions, not total threads. A workload whose
calls block indefinitely still gets one worker per blocked call, because that
is what keeps queued source, sink, and coordinator tasks starting; what the
budget removes is the thread every slow call used to add permanently. Both the
English and Chinese hybrid and separated guides say this.
**Tests**: `TaskExecutionServiceCooperativeBudgetTest` now has two tests,
both run 3x locally with no flakes (~3-6s each). I verified the new regression
actually bites by disabling `ensureQueueIsServed()` locally: it fails with "the
queued task never started, so denied promotions starved the shared queue" after
91s, and passes with the guard. `TaskExecutionServiceTest` (15),
`CooperativeWorkerBudgetTest` (7), `YamlSeaTunnelConfigParserTest` (6) all
pass, and `verify -DskipTests` on both engine modules is clean after spotless.
**On the CI failures on the previous commit**: none are in the changed path,
and all are container or dependency-resolution failures on the fork runner.
- `unit-test (8, ubuntu)`: `Could not resolve dependencies for project
connector-cdc-oracle` - artifact resolution, the build never reached the engine
tests. `unit-test` on JDK 11 and both Windows legs passed, including the engine
module this PR touches.
- `all-connectors-it-7`: `S3FileWithFilterIT` errored in 1.5s;
`all-connectors-it-2`: `IcebergSourceIT` errored in 2.2s;
`all-connectors-it-6`: `DatabendIT` / `DatabendCDCSinkIT` errored in 8-12s -
all container startup errors, not assertion failures.
- `transform-v2-it-part-1`:
`TestFilterRowKindIT#testFilterRowKindMultiTable` expected 0 but was 1, which
is the known multi-table assert-counter flake tracked in #12116.
Cooperative thread sharing is also off by default
(`task_execution_thread_share_mode: OFF`), so none of those suites exercise the
promotion path at all. The new push should give a clean signal; happy to dig
into anything that still fails.
--
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]
