DanielLeens commented on PR #11203:
URL: https://github.com/apache/seatunnel/pull/11203#issuecomment-5340766076
From Daniel's side, wearing the author hat again: this is a fresh,
independent re-review of my own PR, run against the current head
`46be7af12fd3`. I re-derived every claim below from the source at this head
rather than replaying my 2026-08-16 notes from memory, and I want to be upfront
about what actually changed since then versus what didn't.
**What changed since 2026-08-16: nothing in the diff, but CI did.**
- The diff is byte-identical: `git diff` of `RestApiHttpsTest.java` between
the head I reviewed on 2026-08-16 and this head is empty, and the PR-only
compare against `dev` is still exactly one file, `+74/-42` (confirmed via
`repos/apache/seatunnel/compare/dev...46be7af12fd39cff6cd141090acb90e504ff8c45`).
So all nine findings from my 2026-08-16 round are carried forward below as
re-verified, not re-discovered — none of them is new to this round.
- What *is* new: the fork's `Build` run (`31792622595`) has been updated
since I last looked. The three jobs that were previously failing/cancelled at
this exact head — `unit-test (8, windows-latest)`, `unit-test (8,
ubuntu-latest)`, and `all-connectors-it-1 (8/11, ubuntu-latest)` — were rerun
on 2026-08-17/18 and all now show `success`. The overall `Build` conclusion for
this head is now `success`, and the apache-side pointer check reflects the
same. This is the one blocker my last round called out explicitly (`CI gate:
currently FAILURE and must go green before merge`), and it has cleared without
any source change.
# What Problem Does This PR Solve?
- User pain: `RestApiHttpsTest` was environment-fragile in three independent
ways: (a) four hard-coded ports (`28080/28443/28088/28543`) that collide with
anything else on a shared runner, (b) keystore/truststore paths built from
`System.getProperty("user.dir")`, which breaks whenever the working directory
isn't the module root, and (c) REST assertions fired the instant an in-memory
metrics counter reached the expected value, even though the paginated REST view
is backed by a *different* data structure that is populated slightly later.
- Fix approach: replace the fixed ports with `ServerSocket(0)`-discovered
ports, resolve TLS fixtures through the classloader instead of `user.dir`, and
wrap the status-sensitive `/running-jobs` and out-of-range `/finished-jobs`
assertions in a bounded Awaitility poll of the real HTTP response. Also
de-duplicate the success/error read paths into one `readResponseBody` helper
that tolerates a null `getErrorStream()`.
- One-sentence summary: the direction is right and the wait is a genuine
condition-based poll rather than a disguised sleep, but the same race is left
unfixed on the structurally identical `/finished-jobs` path, the poll still
aborts on non-assertion exceptions, and the new port helper reinvents — and
weakens — a hardened allocator that already exists two classes away in the same
package.
# 1. Code Change Review
## 1.1 Core Logic Analysis
I re-read the full file at this head (`RestApiHttpsTest.java`, 410 lines)
rather than trusting the diff alone. Representative before/after for the
response-read path:
Before:
```java
if (conn.getResponseCode() != 200) {
try (BufferedReader in = new BufferedReader(new
InputStreamReader(conn.getErrorStream()))) { ... }
finally { conn.disconnect(); }
} else { ... }
```
After (`RestApiHttpsTest.java:300-324`):
```java
try {
int responseCode = conn.getResponseCode();
String response = readResponseBody(conn, responseCode); // null stream
-> ""
if (callback != null) { callback.callback(responseCode, response); }
} finally {
conn.disconnect();
}
```
Real improvement: the old code NPE'd if `getErrorStream()` returned `null`
(e.g. a 400 with no body); the new code normalizes to `""`.
**Root-cause verification, re-derived independently at this head (not
assumed from the PR description):**
- *Server-not-listening is not the race here.* `SeaTunnelServer.java`
constructs `JettyService` and calls `createJettyServer()` synchronously inside
the Hazelcast node-startup path, and `JettyService.createJettyServer()` ends in
a synchronous `server.start()` at `JettyService.java:260`. By the time
`before()` (`:73-97`) or `getSeatunnelServer()` (`:352-374`) returns, the port
is already bound. A first-connection `ConnectException` is not the mechanism
this PR is fixing.
- *The real remaining race is REST-view lag.*
`getRunningJobMetrics()`/`getJobCountMetrics()` and the paginated REST
endpoints (`/running-jobs`, `/finished-jobs`) are backed by different in-memory
structures populated at different points of the same job-completion sequence,
so the metrics counter used by the pre-existing `await()` can be correct while
the HTTP view still lags by a few hundred milliseconds.
`awaitRestApiRequestHttp` (`:294-298`) correctly targets exactly this gap by
polling the real HTTP response instead of a proxy counter — this is
condition-based polling, not a disguised sleep, and there is no `Thread.sleep`
anywhere in the file.
- *But the fix is not applied uniformly.* `testRunningJobsApi` (`:240`,
`:252`) and `testPageNumberOutOfRange` (`:285`) were migrated to
`awaitRestApiRequestHttp`. `testFinishedJobsApi` (`:181`, `:193`, `:205`) still
calls the plain, non-awaited `restApiRequestHttp` immediately after the same
shape of metrics-count `await()` (`:168-177`), for the same `/finished-jobs`
endpoint that `testPageNumberOutOfRange` *does* wait on. That asymmetry is
Issue 4 below.
**New verification this round: the port helper reinvents an existing
hardened utility.** `RestApiHttpsTest.randomAvailablePort()` (`:385-392`) opens
`new ServerSocket(0)`, closes it, and returns the port — a textbook TOCTOU
window, and because each of the four calls is a fully independent open/close,
the kernel can hand the same ephemeral port to two of the four fields. I
checked whether a better primitive already exists in this package, and it does:
`TestUtils.getAvailablePort(int)`
(`seatunnel-engine-server/src/test/java/.../TestUtils.java:74-95`) is
`synchronized`, tracks every allocation in an in-process
`ALLOCATED_PORT_RANGES` list, and re-verifies bindability via
`isAvailablePortRange`/`isBindablePort` before returning — it structurally
cannot hand out a duplicate within the same JVM, and it closes most of the
TOCTOU window via the bindability recheck. This test's own parent class already
uses it: `AbstractSeaTunnelServerTest.java:59`, `private final int
hazelcastPort =
TestUtils.getAvailablePort(100);`. `TestUtils` is already imported in this
file (`RestApiHttpsTest.java:30`) — it's used for
`TestUtils.getClusterName(...)` and `TestUtils.createTestLogicalPlan(...)` a
few lines below the private helper it could have reused for ports.
## 1.2 Compatibility Impact
**Fully compatible.** The verified PR-only diff (via GitHub's
`compare/dev...head`, not a raw two-dot diff which would also show 28 unrelated
commits of drift) touches exactly one file, under `src/test/java`. No
production API, SPI, configuration option, default value, REST protocol,
serialization format, or checkpoint/restore behavior is touched. No
`docs/en/introduction/concepts/incompatible-changes.md` entry is needed.
## 1.3 Performance / Side-Effect Analysis
- Test-only cost: four short-lived `ServerSocket` binds at instance
construction, plus bounded polling (60s cap / 200ms interval) replacing
immediate one-shot assertions. No `Thread.sleep`. No production runtime,
memory, GC, or concurrency impact — the whole change lives in `src/test/java`.
- Connection hygiene is improved: `restApiRequestHttp` now calls
`conn.disconnect()` in a single `finally` covering both success and error paths
(`:300-311`), where the old code duplicated that logic across two branches.
- Real side effect worth naming: `shutdown(jobInformation)` (`:213`, `:260`,
`:291`) sits at the end of each of the three job-API test method bodies, not in
a `finally`/`@AfterEach`. If any of them throws before reaching it — including
a 60s Awaitility timeout — the second Hazelcast/Jetty instance is leaked with
`httpPort2` still bound, and the *next* of the three tests deterministically
fails to bind that port. This is pre-existing (the old fixed-port code had the
same gap), but the new 60s wait means a leak is now preceded by a full minute
of stall, and a PR whose whole purpose is de-flaking this class is the natural
place to close it. (Issue 5 below.)
## 1.4 Error Handling and Logging
`readResponseBody`'s null-stream guard (`:318-319`) is correct —
`HttpURLConnection.getErrorStream()` legitimately returns `null` for an error
response with no body, and the old code would have NPE'd. `getPath`'s
`IllegalStateException` (`:109-111`) preserves the cause and names the missing
resource. Assertions themselves carry no diagnostic context (Issue 7), which
matters more now that failures can arrive up to 60s late.
Formal issues, severity-sorted (all are carryover from earlier rounds —
dated below — independently re-verified against this exact head, not newly
introduced by any recent commit; issue numbering matches my 2026-08-16 round
for continuity since the code is unchanged):
**Issue 1 (Medium) — `randomAvailablePort()` reinvents, and is weaker than,
the existing `TestUtils.getAvailablePort(int)`**
- Location: `RestApiHttpsTest.java:385-392` (helper), `:68-71` (four call
sites)
- Problem: TOCTOU (port released before the server binds it — `httpPort2` is
drawn at construction but first bound only when the first job-API test runs, a
window of seconds to minutes) plus a duplicate-port risk (each of the four
allocations independently opens-and-closes a socket, so two fields can receive
the same kernel-assigned port). `TestUtils.getAvailablePort(int)` already
closes both gaps in-process and is already used by this test's own parent class.
- Potential risk: a duplicate `httpPort == httpsPort` makes `JettyService`
try to add two connectors on one port, and the synchronous `server.start()`
throws, failing the entire test class — the exact class of flake this PR exists
to remove.
- Best improvement: replace the four `randomAvailablePort()` calls with
`TestUtils.getAvailablePort()`. Four-line change, no new concepts, deletes
Issue 2 as a side effect.
- First raised by: @SEZ9, 2026-07-26 (TOCTOU/duplicate half); the
`TestUtils` reuse specifically was first identified in my 2026-08-16 round.
**Issue 2 (Low) — `socket.setReuseAddress(true)` is called after the socket
is already bound and has no defined effect**
- Location: `RestApiHttpsTest.java:387`
- `new ServerSocket(0)` at `:386` binds immediately; `setReuseAddress` must
be set on an unbound socket before `bind()` to have any effect. The line is a
no-op that reads as intentional hardening but isn't.
- Best improvement: remove it, or adopt Issue 1's fix, which deletes this
line anyway.
- First raised by: Daniel, 2026-08-16.
**Issue 3 (Medium) — `awaitRestApiRequestHttp` retries only
`AssertionError`; transient `IOException`s and JSON parse/cast failures abort
the wait immediately**
- Location: `RestApiHttpsTest.java:294-298`
- `Awaitility.untilAsserted` retries `AssertionError` only. Two concrete
escapes: (a) `conn.getResponseCode()` can throw `IOException` on a reset while
a neighboring test's server is shutting down; (b) the callbacks cast
`Json.parse(content)` to `JsonObject`/`JsonArray` (`:185`, `:197`, `:209`,
`:244`, `:256`) — a truncated or empty body mid-poll throws a
`RuntimeException`/`ClassCastException`, which is *not* an `AssertionError` and
is not retried either. (b) is squarely inside the exact eventual-consistency
symptom this helper exists to poll through.
- Best improvement: add `.ignoreExceptionsInstanceOf(IOException.class)` (or
`.ignoreExceptions()` given this is a bounded test helper) to the Awaitility
chain.
- First raised by: @SEZ9, 2026-07-26 (`IOException` half); the JSON
parse/cast escape was added in my 2026-08-16 round.
**Issue 4 (Medium) — `testFinishedJobsApi` still asserts the paginated
`/finished-jobs` view through the non-awaited helper, the same race class this
PR fixes for `/running-jobs`**
- Location: `RestApiHttpsTest.java:181-212` (three call sites: `:181`,
`:193`, `:205`)
- Traced independently in 1.1: the same metrics-vs-REST-view lag that
motivated converting `testRunningJobsApi` to `awaitRestApiRequestHttp` also
applies here — `getJobCountMetrics().getFinishedJobCount()` and
`/finished-jobs` are backed by different structures populated at different
points. `testPageNumberOutOfRange`, which hits the same endpoint, was already
migrated (`:285`), which makes the omission in `testFinishedJobsApi` look like
an oversight rather than a deliberate choice.
- Best improvement: route the three `testFinishedJobsApi` calls through
`awaitRestApiRequestHttp`, mirroring `:285`. Mechanical, no new concepts.
- First raised by: Daniel, 2026-08-10 (not raised by any other reviewer).
**Issue 5 (Medium) — `shutdown(jobInformation)` is not failure-safe, so a
failure leaks the second cluster and cascades a `BindException` into the next
test**
- Location: `RestApiHttpsTest.java:213`, `:260`, `:291` (helper at
`:376-383`)
- Detailed under 1.3 above. `shutdown` itself is already null-safe, so it's
safe to call unconditionally from a `finally`.
- Best improvement: `try { ... } finally { shutdown(jobInformation); }`
around each of the three test bodies, or move the second-cluster lifecycle to
`@BeforeEach`/`@AfterEach`.
- First raised by: Daniel, 2026-08-16 (not raised by any other reviewer).
Pre-existing behavior, not introduced by this PR, but this PR already touches
every one of these three methods.
**Issue 6 (Low) — `readResponseBody` decodes with the platform default
charset**
- Location: `RestApiHttpsTest.java:321`
- `new InputStreamReader(responseStream)` uses the JVM default charset, not
the UTF-8 the REST API emits. Both CI lanes that execute this test are
`ubuntu-latest` (UTF-8 default), so this is a developer-machine/future-runner
concern rather than a live CI failure today.
- Best improvement: `new InputStreamReader(responseStream,
StandardCharsets.UTF_8)`.
- First raised by: @SEZ9, 2026-07-26.
**Issue 7 (Low) — assertion failures carry no diagnostic context, which
matters more now that failures can arrive up to 60s late**
- Location: `RestApiHttpsTest.java:288-289`, also `:186-190`, `:245-249`
- A bare `Assertions.assertTrue(content.contains(...))` reports only
`expected: <true>` after a full 60s `ConditionTimeoutException`, with no
response code or body.
- Best improvement: `Assertions.assertTrue(content.contains(...), "code=" +
code + ", body=" + content)`.
- First raised by: @SEZ9, 2026-07-26.
**Issue 8 (Low) — `httpsPort2` is allocated and configured but never bound**
- Location: `RestApiHttpsTest.java:71`, `:363-364`
- `getSeatunnelServer` sets `httpConfig.setHttpsPort(httpsPort2)` but
`setEnableHttps(false)`, so no HTTPS connector is ever created for the second
cluster. The draw is dead weight that also widens Issue 1's duplicate-port
surface by one extra allocation.
- Best improvement: drop the field, or add a one-line comment stating it's
intentionally unused.
- First raised by: Daniel, 2026-07-26.
**Issue 9 (Low) — the three new/rewritten helpers carry no explanatory
comments despite non-obvious semantics**
- Location: `RestApiHttpsTest.java:294-298`, `:313-324`, `:385-392`
- The only comment added by the diff is on the `testRunningJobsApi` call
site (`:238-239`), not on the helpers themselves. `awaitRestApiRequestHttp`'s
"only `AssertionError` is retried" contract (Issue 3) and
`randomAvailablePort`'s TOCTOU caveat (Issue 1) are exactly the kind of
non-obvious behavior worth one line of Javadoc each, per the project's
comment-completeness convention for new non-trivial methods.
- First raised by: Daniel, 2026-08-04.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Title follows the `[Test][Zeta]` convention. Helper naming and Awaitility
usage are consistent with the existing style at `:168-177` and `:227-236`.
Imports are explicit, no wildcards. The ASF license header is intact. Gap:
Issue 9 above (missing comments on the three new/rewritten helper methods).
## 2.2 Test Coverage and Test Stability
Coverage scope is unchanged in shape: the same six test methods cover HTTP,
HTTPS, HTTPS-handshake-failure, finished-jobs pagination, running-jobs
pagination, and the out-of-range page path. No assertion was removed, no
tolerance widened, no test disabled or skipped, and no exception type loosened
relative to the merge base — the strictness delta is zero or positive (the
error-path read is now exercised through one consolidated helper instead of a
duplicated branch that could NPE on a null error stream).
**Mandatory stability analysis (Section 5.10.2):**
- Anti-patterns checked and *not* present: no `Thread.sleep`/hard waits
anywhere in the file; every new or touched wait is a bounded
`Awaitility.await().atMost(60, SECONDS).untilAsserted(...)` against a real,
observable condition (either the pre-existing metrics-count check or, new in
this PR, the actual HTTP response); no shared static mutable state; no
floating-point comparisons; no new order-sensitivity (each job-API test builds
and tears down its own Hazelcast+Jetty cluster); no reliance on `HashMap`
iteration order.
- Anti-patterns present, with evidence (these are Issues 1, 3, 4, 5 above):
port-allocation TOCTOU/duplicate risk at `RestApiHttpsTest.java:385-392`;
Awaitility retrying `AssertionError` only, so transient
`IOException`/JSON-parse failures abort the poll at `:294-298`; the
`/finished-jobs` race left unfixed at `:181-212`, structurally identical to the
race fixed for `/running-jobs`; and non-`finally` cluster teardown at
`:213/:260/:291` that turns one flaky assertion into a cascading multi-test
failure.
**Stability rating: Risk present.**
Rationale: none of the four residual vectors is High risk — each requires a
specific, comparatively low-probability trigger (a connection reset during a
neighboring test's shutdown, a genuinely truncated body mid-poll, or another
process racing a freshly-closed ephemeral port), and the change is strictly
better than the pre-PR baseline (fixed ports + `user.dir` paths + zero retry)
on every axis. It is not Stable, because four independent, evidenced vectors
survive in code whose sole purpose is eliminating exactly this class of flake,
and one of them (Issue 5) can amplify a single flaky assertion into three
failing tests plus a leaked Hazelcast instance. Per Section 5.10.2(c), a
Medium-severity "Risk present" item is non-blocking but must carry a
remediation recommendation, which each issue above does.
## 2.3 Documentation Updates
Not applicable. No user-visible behavior, configuration option, or default
value changes; correctly, no `docs/en`/`docs/zh` edits are present or required.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Precise fix**, at the correct boundary — the test itself, not production
code. It replaces three environmental assumptions (fixed ports,
`user.dir`-relative paths, timing luck) with observable signals (classpath
resolution, kernel-assigned ports, polling the actual endpoint under
assertion). The one place it reaches for the obvious thing instead of the
correct one is the port helper (Issue 1), where a hardened equivalent already
exists two classes away.
## 3.2 Maintainability
Consolidating the duplicated success/error read branches into
`readResponseBody` removes a copy-paste hazard. `awaitRestApiRequestHttp` gives
future REST assertions in this class a single seam to gain retry semantics —
which is exactly what makes closing Issue 3 and Issue 4 cheap and mechanical.
## 3.3 Extensibility
The await/request/read helpers are directly reusable for any new REST
assertion added to this class. A genuinely long-term fix for the port TOCTOU
class — having `JettyService` bind port 0 and expose the actually-bound port
back to callers — needs production-code plumbing (`JettyService` today has no
such accessor) and is legitimate separate follow-up, not a reason to hold this
PR.
## 3.4 Historical-Version Compatibility
Not affected. Test-only change; no serialized formats, checkpoint layouts,
config defaults, or REST contracts are touched, so there is no
historical-version migration concern.
# 4. Issue Summary
| # | Issue | Location | Severity |
|---|---|---|---|
| 1 | `randomAvailablePort()` reinvents and weakens the existing
`TestUtils.getAvailablePort(int)`; TOCTOU plus duplicate-port risk |
RestApiHttpsTest.java:385-392, 68-71 | Medium |
| 2 | `setReuseAddress(true)` called after bind, no defined effect |
RestApiHttpsTest.java:387 | Low |
| 3 | `untilAsserted` retries only `AssertionError`; `IOException` and JSON
parse/cast failures abort the wait | RestApiHttpsTest.java:294-298 | Medium |
| 4 | `testFinishedJobsApi` still uses the non-awaited helper for the same
race class already fixed elsewhere in this PR | RestApiHttpsTest.java:181-212 |
Medium |
| 5 | `shutdown(...)` not in `finally`/`@AfterEach`; a failure leaks the
cluster and cascades a `BindException` into the next test |
RestApiHttpsTest.java:213,260,291 | Medium |
| 6 | `readResponseBody` decodes with the platform default charset |
RestApiHttpsTest.java:321 | Low |
| 7 | No assertion diagnostics; failures can now arrive up to 60s late with
no context | RestApiHttpsTest.java:288-289 | Low |
| 8 | `httpsPort2` allocated and configured but never bound |
RestApiHttpsTest.java:71,363-364 | Low |
| 9 | New helpers carry no explanatory comments |
RestApiHttpsTest.java:294-298,313-324,385-392 | Low |
# 5. Merge Recommendation
### Conclusion: Ready to merge after fixes
1. **Blockers — must be fixed**
- None on source correctness, compatibility, or diff integrity. All nine
issues above are non-blocking on the code itself: none makes this test flakier
than the fixed-port/`user.dir`/zero-retry baseline it replaces, and the
strictness delta versus the merge base is zero or positive.
- **Process gate, now cleared:** the required `Build` check was `FAILURE`
at this exact head as of my 2026-08-16 round, driven by three jobs unrelated to
`RestApiHttpsTest` (an unrelated Windows Hazelcast "Node failed to start!"
flake in a different test class, and an unrelated Postgres CDC IT). I
re-checked the fork's actual run (`31792622595`) today: all three of those jobs
were rerun on 2026-08-17/18 and are now `success`, and the overall `Build`
conclusion for this head is `success`. So the one concrete blocker from my last
round no longer applies. `RestApiHttpsTest` itself has completed successfully
on every lane where it actually executes (`unit-test 8/11, ubuntu-latest`; it
is skipped on Windows via its own `@DisabledOnOs`).
- The PR is still marked **draft** and has no maintainer
`APPROVED`/`CHANGES_REQUESTED` review — @SEZ9's 2026-07-26 pass and every one
of my rounds were comments, not formal reviews, since GitHub blocks
self-approval on an author's own PR and my own account has read-only repository
access here. A write-capable maintainer still needs to perform the final
review/approval/merge step; from Daniel's side there is no remaining
source-level blocker.
2. **Recommended fixes — non-blocking** (I'd suggest bundling these into one
follow-up commit before final sign-off, since a "stabilize this test" PR that
ships with an already-diagnosed instance of the exact race it fixes elsewhere
in the same file undercuts its own premise, and together they're roughly
fifteen lines):
- Issue 1: swap `randomAvailablePort()` for the existing
`TestUtils.getAvailablePort()` — highest value per line, also removes Issue 2
and shrinks Issue 8's blast radius.
- Issue 4: wrap the three `testFinishedJobsApi` calls in
`awaitRestApiRequestHttp`, matching `testPageNumberOutOfRange`.
- Issue 3: add `.ignoreExceptionsInstanceOf(IOException.class)` to the
Awaitility chain.
- Issue 5: move `shutdown(jobInformation)` into a `finally` block (or
`@AfterEach`).
- Issues 2, 6, 7, 8, 9: cheap one-line cleanups; fine to fold in while
touching the file.
**Overall assessment:** the stabilization approach is correct and genuinely
condition-based rather than a sleep-widen — I independently re-verified that
Jetty binds synchronously (so "server not listening" isn't the trigger) and
that the metrics-vs-REST-view lag is real and exactly what
`awaitRestApiRequestHttp` targets. What holds it back from "ready to merge
as-is" is that the fix isn't applied uniformly within its own file
(`/finished-jobs` is left on the old, non-awaited path) and the new port helper
duplicates, and weakens, an allocator that already exists and is already used
by this test's own parent class. None of that is a source blocker; all of it is
worth closing given the PR's stated purpose. With CI now green at this head,
the remaining step from my side is a write-capable maintainer's final review —
happy to push the five bundled fixes above first if that's preferred before
requesting it.
--
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]
