DanielLeens commented on PR #11203:
URL: https://github.com/apache/seatunnel/pull/11203#issuecomment-5379380811
# What Problem Does This PR Solve?
`RestApiHttpsTest` (in `seatunnel-engine-server`) has historically flaked in
CI for a few independent reasons: it bound to four hardcoded ports
(`28080`/`28443`/`28088`/`28543`) that can collide with other suites, it
resolved its keystore/truststore fixtures via a `user.dir`-relative path that
breaks when the working directory isn't the module root, and its paginated
`/running-jobs` REST assertions could run before the REST view had caught up
with the in-memory job-metrics map that the test itself had just waited on.
The fix approach is to replace the four hardcoded ports with per-instance
dynamically allocated ports, resolve the keystore/truststore fixtures through
the classloader instead of `user.dir`, wrap the running-jobs and
out-of-range-page REST checks in an `Awaitility` retry loop, and consolidate
the duplicated success/error response-reading branches into one
`readResponseBody` helper.
One-line summary: directionally correct and entirely test-only, but — as I
already flagged across six prior review rounds on this same PR, and as @SEZ9
independently found in a parallel review — several of the very reliability gaps
this PR sets out to close are still present in the code as of the current head,
so the "stabilize" goal is only partially delivered.
I'm the author of this PR as well as the reviewer here. Per the review
protocol I'm treating this as an honest independent maintainer pass, not a
rubber stamp — GitHub also blocks self-approval on this PR, so nothing here
carries approval weight regardless.
# 1. Code Change Review
## 1.1 Core Logic Analysis
**Port allocation, before/after:**
```java
- private static final int HTTP_PORT = 28080;
- private static final int HTTPS_PORT = 28443;
- private static final int HTTP_PORT2 = 28088;
- private static final int HTTPS_PORT2 = 28543;
+ private final int httpPort = randomAvailablePort();
+ private final int httpsPort = randomAvailablePort();
+ private final int httpPort2 = randomAvailablePort();
+ private final int httpsPort2 = randomAvailablePort();
...
+ private int randomAvailablePort() {
+ try (ServerSocket socket = new ServerSocket(0)) {
+ socket.setReuseAddress(true);
+ return socket.getLocalPort();
+ } catch (Exception e) {
+ throw new IllegalStateException("Failed to allocate a free test
port", e);
+ }
+ }
```
**New retry wrapper, used for `/running-jobs` and the out-of-range
`/finished-jobs` page check, but not for the rest of `/finished-jobs`:**
```java
+ private void awaitRestApiRequestHttp(String url, RestApiRequestCallback
callback) {
+ await().atMost(60, TimeUnit.SECONDS)
+ .pollInterval(200, TimeUnit.MILLISECONDS)
+ .untilAsserted(() -> restApiRequestHttp(url, callback));
+ }
```
**Teardown, unchanged in structure — still a plain last statement:**
```java
awaitRestApiRequestHttp(... "/finished-jobs?page=10&rows=" +
pageSize ..., ...);
shutdown(jobInformation); // RestApiHttpsTest.java:213 / 260 / 291
— not try/finally, not @AfterEach
}
```
**Key Findings**
- `randomAvailablePort()` duplicates, less safely, functionality this same
module already has: `TestUtils.getAvailablePort(int)` (`TestUtils.java:74`) is
`synchronized`, validates the port against a static `ALLOCATED_PORT_RANGES`
list to avoid handing out a port another test class already claimed, and
re-verifies bindability before returning. It is already used by this test's own
parent class (`AbstractSeaTunnelServerTest.java:59`) and by every sibling REST
test class (`OptionRulesApiTest`, `RestApiSubmitJobConfigShadeDecryptTest`,
`RestApiSubmitJobStartWithSavePointTest`) for their Hazelcast port.
`randomAvailablePort()` here bypasses that registry entirely in both
directions: it never registers what it picks, and it never checks what's
already registered — see Issue 1.
- `testFinishedJobsApi()` still calls the plain `restApiRequestHttp` (not
the new `awaitRestApiRequestHttp`) for all three of its `/finished-jobs`
checks, even though it waits on the exact same kind of in-memory metric
(`getFinishedJobCount()`) before querying the REST endpoint that
`testRunningJobsApi()` does — and that second one *was* switched to the awaited
variant. See Issue 2.
- `Awaitility.untilAsserted(...)` only retries on `AssertionError`;
`restApiRequestHttp` is declared `throws Exception` and can throw `IOException`
(e.g. `ConnectException` while Jetty is still binding) or a JSON-parsing/cast
exception, either of which escapes the retry loop on the very first poll. See
Issue 3.
- `shutdown(jobInformation)` still runs only as the last statement of each
test method, not in a `finally`/`@AfterEach`. See Issue 4.
**Runtime path — does the normal path reach every changed line?**
Yes, this is straight-line mainline test flow, not a boundary path:
```text
RestApiHttpsTest instance construction (JUnit5 default: new instance per
@Test method)
-> httpPort/httpsPort/httpPort2/httpsPort2 = randomAvailablePort() x4
RestApiHttpsTest.java:68-71
-> new ServerSocket(0) -> getLocalPort() -> close()
RestApiHttpsTest.java:385-391
[port released back to the OS before any server binds it - Issue 1]
@BeforeAll before()
RestApiHttpsTest.java:76
-> httpConfig.setPort(httpPort) / setHttpsPort(httpsPort)
-> super.before() -> SeaTunnelServer binds Jetty on httpPort/httpsPort
testFinishedJobsApi() / testRunningJobsApi() / testPageNumberOutOfRange()
-> getSeatunnelServer(...) -> new local HazelcastInstanceImpl bound to
httpPort2/httpsPort2
-> startJob(...) x N -> CoordinatorService.submitJob(...)
-> await() on coordinatorService.getFinishedJobCount() /
getRunningJobMetrics() (pre-existing, in-memory)
-> [running-jobs, out-of-range page] awaitRestApiRequestHttp(url, cb)
RestApiHttpsTest.java:240,252,285
-> Awaitility.untilAsserted(() -> restApiRequestHttp(...))
RestApiHttpsTest.java:294-298
[retries AssertionError only - Issue 3]
-> [finished-jobs pages 1/2/no-pagination] restApiRequestHttp(url, cb)
DIRECTLY, no await wrapper RestApiHttpsTest.java:179-208
[same REST-view-lag race the PR fixes for running-jobs, left open
here - Issue 2]
-> shutdown(jobInformation) RestApiHttpsTest.java:213/260/291 (end of
method body, not finally - Issue 4)
```
## 1.2 Compatibility Impact
**Fully compatible.** Test-only file, no production code touched, no
`Option`/config/protocol/serialization change, nothing that affects a running
cluster, checkpoint, or historical job.
## 1.3 Performance / Side-Effect Analysis
Negligible: four extra ephemeral `ServerSocket` open/close pairs at
test-instance construction, and up to 60s of additional polling budget per REST
assertion (bounded, and only consumed on genuine delay). No new locking, no
retries beyond the intended Awaitility wrapper, no resource leak in the changed
lines themselves besides the pre-existing/newly-more-visible `shutdown()` gap
in Issue 4.
## 1.4 Error Handling and Logging
`readResponseBody` correctly handles a `null` `getErrorStream()` (returns
`""`) where the old code would have NPE'd on some error responses — a genuine
improvement. Remaining gaps are captured as Issues 3, 5 and 6 below.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Consistent with the surrounding file; the new helpers are private and
reasonably named. No missing license header (existing file). See Issue 4 for
the one structural gap (resource cleanup not guaranteed).
## 2.2 Test Coverage and Test Stability
This PR *is* a test-stability change, so the mandatory flaky-test analysis
applies directly:
- No raw `Thread.sleep`; `Awaitility` is used for all new waits.
- No floating-point assertions.
- No new shared static mutable state (ports are instance fields;
`TestUtils.ALLOCATED_PORT_RANGES` — the one piece of real shared static state
relevant here — is exactly what this PR's port picker bypasses, per Issue 1).
- Order-dependence: not introduced between this class's own tests (each gets
fresh random ports), but Issue 1 creates a *cross-class* order-dependence risk
against `TestUtils.getAvailablePort()` users elsewhere in the suite.
- Weak retry coverage: Issue 3 (IOException escapes `untilAsserted`) is a
genuine, previously-identified-and-still-unfixed gap in exactly the kind of
polling helper this PR introduces to fix flakiness.
- Issue 2 leaves one of the three REST-view-lag-prone call sites completely
unguarded despite the sibling call sites being fixed in the same PR.
None of this currently manifests as an observed failure — CI on the current
head (`46be7af12fd3`) is green — so I'm not calling this "High risk" (that
would require concrete reproducing evidence of a currently-failing run). But
the gaps are real, specific, and would each independently explain a future
flake if hit, which is precisely the category of bug this PR exists to
eliminate.
**Rating: Risk present** (path:line evidence: Issue 1 at
`RestApiHttpsTest.java:385-391`, Issue 2 at `RestApiHttpsTest.java:179-208`,
Issue 3 at `RestApiHttpsTest.java:294-298`).
## 2.3 Documentation Updates
Not applicable — no `Option`, no user-facing config or behavior change.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Directionally right, incompletely executed.** The classloader-based
resource resolution and the `readResponseBody` consolidation are clean, precise
fixes. The port allocation and the retry-wrapper rollout are each a reasonable
idea implemented with a gap that undercuts the stated goal (a second, weaker
port allocator instead of reusing the hardened one already in this module; a
retry wrapper applied to two of three structurally-identical call sites).
## 3.2 Maintainability
Once Issue 1 is fixed (delegate to `TestUtils.getAvailablePort`), this test
becomes strictly easier to maintain than the hardcoded-port version it
replaces. The duplicated-then-consolidated `readResponseBody` is also a
maintainability win as-is.
## 3.3 Extensibility
N/A — test-only stabilization, not new functionality.
## 3.4 Historical-Version Compatibility
No impact — test-only, nothing persisted or exposed to users.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|-------|----------|----------|
| 1 | `randomAvailablePort()` reinvents a weaker, non-synchronized,
non-registry-tracked port picker instead of the already-hardened
`TestUtils.getAvailablePort(int)` this module's own parent class and every
sibling REST test class use | `RestApiHttpsTest.java:385-391` | Medium |
| 2 | `testFinishedJobsApi()`'s three `/finished-jobs` checks still use the
non-awaited `restApiRequestHttp`, unlike the structurally identical
`/running-jobs` and out-of-range `/finished-jobs` checks in the same file |
`RestApiHttpsTest.java:179-208` | Medium |
| 3 | `awaitRestApiRequestHttp`'s `untilAsserted` only retries
`AssertionError`; a transient `IOException`/`ConnectException` during server
warm-up escapes on the first poll instead of retrying |
`RestApiHttpsTest.java:294-298` | Medium |
| 4 | `shutdown(jobInformation)` runs as the last statement of each test
method rather than in `finally`/`@AfterEach`, so an assertion failure above it
leaks the local Hazelcast instance for the rest of the suite run |
`RestApiHttpsTest.java:213, 260, 291` | Medium |
| 5 | `readResponseBody` decodes with the platform default charset instead
of explicit UTF-8, which can garble non-ASCII content and cause spurious
assertion failures on non-UTF-8-default CI images | `RestApiHttpsTest.java:321`
| Low |
| 6 | REST-callback assertions (e.g. the "Page number exceeds total pages"
check) carry no failure message, so a real failure after the new 60s wait
surfaces as a bare `expected: true` with no response code/body context |
`RestApiHttpsTest.java:288-289` | Low |
Issues 1 and 3 restate and re-verify my own prior review rounds' findings
against the current head, and overlap with @SEZ9's 2026-07-26 Issue 1 and Issue
2 respectively (still open, unaddressed since that review). Issue 5 restates
@SEZ9's Issue 4 (still open). Issue 6 restates @SEZ9's Issue 3 (still open).
Issue 2 and Issue 4 are new findings from this pass — Issue 2 in particular is
a fresh observation: a third structurally-identical call site was never brought
in line with the other two that this same PR did fix.
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. Blockers — must be fixed
- Issue 1: delegate to `TestUtils.getAvailablePort(int)` instead of the
hand-rolled `randomAvailablePort()`, so this test's ports participate in the
same shared-registry de-duplication every sibling REST test class relies on.
- Issue 2: apply `awaitRestApiRequestHttp` to `testFinishedJobsApi()`'s
three `/finished-jobs` calls, matching the sibling call sites already fixed in
this PR.
- Issue 3: add `.ignoreExceptions()` (or
`.ignoreExceptionsInstanceOf(IOException.class)`) to the
`awaitRestApiRequestHttp` chain so transient connection failures are retried
instead of failing the wait immediately.
- Issue 4: move `shutdown(jobInformation)` into a `finally` block so a
mid-test assertion failure doesn't leak the local Hazelcast instance.
2. Recommended fixes — non-blocking
- Issue 5: decode `readResponseBody` with `StandardCharsets.UTF_8`
explicitly.
- Issue 6: add response code/body context to the REST-callback assertion
messages.
Overall assessment: this PR is a real improvement over the hardcoded-port
baseline and CI is currently green, but as author I don't think it's honest to
call it fully "stabilized" while it still contains a weaker duplicate of an
existing hardened helper and leaves one of its own three analogous call sites
unfixed. None of the four blockers above is large — together they're roughly
the same handful of lines I estimated in my last self-check — and none touches
production code, so there's no reason this can't be wrapped up and taken out of
draft in the same sitting. I'd like a maintainer's eyes on this once those four
are addressed, since neither my own comments nor @SEZ9's carry approval weight
on this PR.
--
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]
