Asthenia0412 commented on PR #12795: URL: https://github.com/apache/seatunnel/pull/12795#issuecomment-6229669266
Hi @DanielLeens, many thanks for your thorough review! Here's what this new push (commit `88703260`) addresses from your latest CHANGES_REQUESTED round: ### Issue 1 (Medium) — Dedicated fallback scheduler The 15 s fallback now runs on its own **single-thread `ScheduledThreadPoolExecutor`** (`finalMetricsFallbackExecutorService`) instead of sharing the periodic metrics-backup `scheduledExecutorService`. The periodic run calls the same un-timed `invoke.get()`, which could previously block the fallback from firing on time — negating the timeout guarantee. The new scheduler uses `setRemoveOnCancelPolicy(true)` and is shut down in `shutdown()`. ### Issue 2 (Low) — Cancel fallback after normal completion We now track the returned `ScheduledFuture<?>` from `schedule()` and call `fallbackFuture.cancel(false)` in the report's `finally` block. Normal completions no longer leave a 15 s-delayed task lingering in the queue. ### Issue 4 (Low) — Style cleanup - `FINAL_METRICS_FALLBACK_TIMEOUT_MS` moved to class-top level (package-private), near other static declarations. - The instance initializer block is gone — pool creation now lives in a **private static factory** (`createFinalMetricsExecutor` / `createFinalMetricsFallbackExecutor`), called from the constructor alongside the other executors. - Replaced the `// Visible for testing` comment with `@VisibleForTesting`. Spotless:apply ran clean, and `mvn clean compile test-compile -pl seatunnel-engine-server` succeeds locally under JDK 17. Please let me know if anything else needs attention! -- 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]
