DanielLeens commented on PR #11203: URL: https://github.com/apache/seatunnel/pull/11203#issuecomment-5342817652
A second independent pass, run minutes after my last comment on this same head (`46be7af12fd3`) — noting up front that this is a corroboration round, not a fresh numbered-issue list, since the prior comment already covers the full Section-structure review at this exact head and nothing in the diff has changed since. # What Problem Does This PR Solve? `RestApiHttpsTest` was environment-fragile in three independent ways: fixed ports that collide on shared runners, `user.dir`-relative keystore paths, and REST assertions that fired the instant an in-memory metrics counter matched, even though the paginated REST view is backed by a different, slightly-later-populated data structure. The fix replaces all three with kernel-assigned ports, classpath-resolved fixtures, and a bounded Awaitility poll of the real HTTP response for the status-sensitive checks. # Independent re-verification of the root cause (adds one piece of evidence beyond the prior round) I traced the `/running-jobs` race down to the actual code paths rather than taking "different structures, different timing" as given: - The pre-existing readiness `await()` in `testRunningJobsApi` polls `CoordinatorService.getRunningJobMetrics()`, which is keyed off `runningJobMasterMap.keySet()` and then does a **live RPC** per worker address — `NodeEngineUtil.sendOperationToMemberNode(..., new GetMetricsOperation(runningJobIds), address)` — resolved through `ownedSlotProfilesIMap` (`CoordinatorService.java:1521-1553`). - The `/running-jobs` REST endpoint (`RunningJobsServlet` → `JobInfoService.getRunningJobsJson()`) instead reads `IMAP_RUNNING_JOB_INFO` directly and filters each entry through `shouldShowAsRunningJob(jobId)` → `CoordinatorService.getJobStatus(jobId)` (`JobInfoService.java:171-178`, `:348-351`). These are two genuinely separate subsystems (a remote per-worker metrics RPC vs. a local IMap read gated by job-status), so the metrics-count condition and the REST view's own internal state can legitimately settle at different times under CI load. `awaitRestApiRequestHttp` polls the second (real) signal directly instead of trusting the first as a proxy, which is the technically correct fix for this specific race — not a blind sleep dressed up as a wait. This same corroboration extends Issue 4 from the prior round: since `testFinishedJobsApi`'s pagination checks still go through the non-awaited `restApiRequestHttp`, and `getJobCountMetrics().getFinishedJobCount()` / the `/finished-jobs` view both trace back to `jobHistoryService` and `IMAP_FINISHED_JOB_STATE` on the same synchronous path (no separate RPC hop the way `/running-jobs` has), the asymmetry is architecturally justified rather than arbitrary — but `testFinishedJobsApi` not being migrated to `awaitRestApiRequestHttp` is still the right non-blocking cleanup called out as Issue 4, for consistency and defense-in-depth even where the race is currently less likely to fire. # CI diagnosis (re-checked live) `gh pr view 11203 --json statusCheckRollup,mergeStateStatus,mergeable` at this moment: `Build` = SUCCESS, `Notify test workflow` = SUCCESS, `labeler` = SUCCESS. `mergeStateStatus` is `BLOCKED` only because this is a draft PR awaiting a write-capable maintainer's formal review/approval — there is no outstanding CI or source-level blocker from my side. # Merge Recommendation ### Conclusion: Ready to merge after fixes Unchanged from the prior round: no blocker on correctness, compatibility, or diff integrity. The nine non-blocking issues already listed (port-helper TOCTOU/duplicate-port risk reinventing `TestUtils.getAvailablePort()`, `Awaitility` not ignoring `IOException`/parse-cast exceptions, `testFinishedJobsApi` left on the non-awaited path, `shutdown(...)` not wrapped in `finally`, plus the smaller charset/comment/dead-field items) remain valid and worth folding into one small follow-up commit before final sign-off. CI is green at this head; the remaining step is maintainer review. -- 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]
