Asthenia0412 commented on PR #12795: URL: https://github.com/apache/seatunnel/pull/12795#issuecomment-6208216146
Hi @DanielLeens — thank you so much for the thorough second round of reviews! I pushed a new commit (328d13ab9) that addresses every item from your latest feedback. The full summary: **Blocker — Issue 1 (Spotless)** - Ran `mvn spotless:apply`; the three `completeAfterFinalMetrics(...)` call sites are now on single lines. Spotless check passes locally. This unblocks the full CI matrix. **Medium — Issue 2 (Uninterruptible / un-timed RPC)** - Added a scheduled fallback on the existing `scheduledExecutorService`: if the final metrics report hasn't returned after **15 s**, the terminal future is completed directly. `CompletableFuture.complete()` is idempotent so the finally-block completion and the fallback race safely (whichever fires first wins). This is Option A from your review — keeps the current design, just adds an upper bound. **Medium — Issue 4a (CANCELED-path ordering test)** - Added `testFinalMetricsPreservesCanceledTerminalState`. It triggers the real cancellation path (`cancellationFuture.completeExceptionally`), drives `taskDone` on a worker whose interrupt flag is set, blocks the mock metrics report mid-way, and asserts (a) metrics runs on a non-worker thread, (b) worker interrupt flag is preserved, (c) the CANCELED future is **not** completed until the mock report is released, and (d) the final terminal state is `CANCELED`. **Medium — Issue 4b (Visibility widened silently)** - Added `// Visible for testing (Mockito spy in TaskExecutionServiceTest).` directly above `updateMetricsContextInImap()`. **Low — Issue 3 (Thread pool hygiene)** - Replaced the anonymous lambda factory with a named `ThreadFactory` that names pool threads `seatunnel.final-metrics-1`, `seatunnel.final-metrics-2`, ... so they are distinguishable in thread dumps. - Wrapped the pool with `MDCTracer.tracing(...)` so the metrics report keeps the job's MDC context instead of dropping it. - Lowered the submit-failure log from `severe` to `warning` with an action-oriented message (the only expected rejection path is shutdown, not a configuration failure). - Softened the Javadoc from "orderly shutdown drains accepted reports" to "best-effort", matching what the pool actually does. **Low — Issue 5 (Context classloader inheritance)** - Each pool thread is born with `TaskExecutionService.class.getClassLoader()` so it does not accidentally inherit a task worker's `SeaTunnelChildFirstClassLoader` (which may have been recycled or belong to a different job by the time the pooled thread is reused). **Verification** - Local: `mvn spotless:apply spotless:check` — green - Local: `mvn test -pl seatunnel-engine/seatunnel-engine-server -Dtest="TaskExecutionServiceTest#testFinalMetrics*"` — **4 tests, all pass** (FINISHED isolation, CANCELED isolation, FAILED metrics-failure preservation, executor-rejection fallback) Fork-side CI is now running against this new head — I'll be happy to triage anything that surfaces there and iterate further. Thanks again! -- 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]
