DanielLeens commented on PR #12137: URL: https://github.com/apache/seatunnel/pull/12137#issuecomment-6411614838
Quick CI follow-up on `cf275319`, because my review from earlier today said the `Build` check was still pending, and that is now out of date. **Current CI state of this exact head:** the apache `Build` check finished as `failure` (https://github.com/apache/seatunnel/runs/121072808517), pointing to the fork run https://github.com/goutamadwant/seatunnel/actions/runs/40161136699. The head is unchanged since my review (`cf275319`). **What failed** 1. `Run / unit-test (17, ubuntu-latest)` failed in `seatunnel-engine-server`: 682 tests, 1 failure and 1 error, both in `RestApiHttpsTest`: - `testFinishedJobsApi`: `expected: <true> but was: <false>` at `RestApiHttpsTest.java:190` (the `/finished-jobs?page=1&rows=5` response check, right after the job-count metric reached 7). - `testRunningJobsApi`: `ConditionTimeout` after 1 minute in `awaitRestApiRequestHttp`. 2. `Run / all-connectors-it-3 (11, ubuntu-latest)` was cancelled at the 150 minute `timeout-minutes` budget (the 17 leg of the same shard passed). **Is it related to the diff?** My read, as an inference from the logs and the diff (nothing was run locally): not caused by this PR's own content. - `RestApiHttpsTest` is not touched by this PR, and the failing assertions are about the REST list views of finished and running jobs, not about CDC progress. The test already carries a comment that the running-jobs REST view can lag the metrics map under CI load, and `testFinishedJobsApi` has the same shape (it waits on the metric count, then reads the REST list once without a retry). - The same test class passed on the other three `unit-test` legs of this run (Ubuntu JDK 11, Windows JDK 11 and 17), and all four `unit-test` legs passed on the base PR #11512, which carries the same engine-side CDC code. - Caveat: this PR does modify `CoordinatorService` and `JobMaster`, so I would not call it unrelated based on file ownership alone. The evidence is the pass/fail pattern across legs plus the timing-dependent assertion, so a single clean re-run of the failed leg would settle it. **Minimal next step:** no code change is needed for this CI result. Since this PR is a draft stacked on #11512, the CI that will count is the one on the rebased head after #11512 lands, so I would not spend a re-run on this head. If you want an earlier signal, "Re-run failed jobs" on the fork run (only `unit-test (17, ubuntu-latest)` and `all-connectors-it-3 (11, ubuntu-latest)`) is enough. If `RestApiHttpsTest` fails again on a clean re-run, please share it here and I will look at it properly. **Source conclusion:** unchanged from my review. `### Conclusion: Not recommended for merge` still stands for `cf275319`, for the one blocker I listed: wait for #11512 to merge, rebase so the diff contains only the three owned files, and mark the PR ready. The branch is now 16 commits behind `dev` with no conflicts; the rebase after #11512 lands will cover that. Nothing was executed locally for this follow-up; it is based on the check-run output and the fork's Actions logs. -- 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]
