DanielLeens commented on PR #11602: URL: https://github.com/apache/seatunnel/pull/11602#issuecomment-5322447661
Thanks @SEZ9 — before replying I re-pulled the current head (`964747f`) directly via the GitHub API to double-check, and I think two things here need correcting: **1. The code changes are already on this PR.** The current head has 14 changed files (367 additions / 106 deletions) covering exactly the asks from our earlier rounds: - `Constant.DEFAULT_METRICS_FETCH_TIMEOUT_MS` plus the new `seatunnel.engine.metrics-fetch-timeout-ms` option, wired through `ServerConfigOptions`, `EngineConfig`, `YamlSeaTunnelDomConfigProcessor`, and both `config/seatunnel.yaml` and `seatunnel-engine-common/.../resources/seatunnel.yaml` (`metrics-fetch-timeout-ms: 3000` is there at line 24 of the latter). - The `SubPlan` retry-predicate comment explaining why `FinalMetricsCollectionException` is retryable is present right above the `RetryMaterial` construction. - `getCurrJobMetrics`/`getFinalJobMetrics` Javadoc updates, the two new `JobMasterTest` cases, and both `docs/en`/`docs/zh` incompatible-changes entries are all there too. So there's nothing outstanding to push — my last full review (8/13) already re-verified all of this against `964747f` file-by-file. **2. On "2 commits behind `dev`"** — I just re-ran `compare/dev...964747f` and it currently reports `ahead_by: 17, behind_by: 63`, not 2. It's drifted further behind since we last looked. On the CI failure itself: `unit-test (11, windows-latest)` is genuinely red on this head, but it's the same pre-existing `MultiTableSinkWriterSchemaChangeBroadcastTest` close()-race flake I flagged in my last review — in `seatunnel-api`, a module this PR doesn't touch at all. `dev` already carries a fix for that exact race (#11726, "Tolerate the pre-existing close() race in the schema-change worker-failure test", merged 8/11). There's also a `rocketmq-connector-it (11, ubuntu-latest)` failure on this head, again in a module this PR doesn't touch. So the concrete next step is a rebase, not new code: since the branch is 63 commits behind and `dev` already contains a fix for the specific flake that's failing here, syncing onto latest `dev` and rerunning CI should clear the Windows unit-test failure outright and give us a clean IT-lane signal too. @tomatotomata once the rebase is done and CI comes back green, I'll take a final pass. -- 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]
