DanielLeens commented on PR #11641:
URL: https://github.com/apache/seatunnel/pull/11641#issuecomment-5342884245
A note up front, same as every prior pass on this thread: this is my own PR,
so this is a self-review and cannot count as an independent approval — GitHub
blocks self-approval, so it is posted as a plain comment. There is no new
commit since my last pass (10:28 UTC today) or since @davidzollo's `APPROVED`
review, both on this exact head (`be6b805`), so the code-level analysis below
is a verification pass, not a review of new code. What has changed is CI: my
own 10:28 UTC comment reported the `Build` check at `conclusion: failure`
(unrelated `all-connectors-it-1` / `jdbc-connectors-it-ddl` IT shards) and
recommended a `dev` sync + rerun. That has since resolved on its own — the
required `Build` check on this exact head now reports `pass` (53m32s, full run,
not a sanity-only short-circuit). I'm correcting the record rather than leaving
stale "wait for sync" guidance standing.
I also independently re-derived the core technical claims below by reading
the actual engine and Awaitility library source rather than trusting the prior
16 rounds' text, and found one genuinely new piece of evidence worth adding
(see 1.3).
# What Problem Does This PR Solve?
- **User pain point:** `MetricsApiTest#metricsApiTest` intermittently failed
CI with `Expected status code <200> but was <500>` on `GET /metrics`, observed
on at least five unrelated PRs (#10874, #10958, #10973, #11060, #11382) on both
JDK 8 and JDK 11 `unit-test` lanes.
- **Fix approach:** replace the single, immediate assertion with a bounded
(60s cap, 1s interval, explicit zero poll-delay) Awaitility poll around a new
`assertMetricsExposed()` helper, add a per-request 5s connect/socket timeout,
convert transport failures into retryable `AssertionError`s instead of letting
them abort the poll, and attach the response body to the failure message.
- **One-sentence summary:** the test encoded an implicit "the metrics
endpoint is fully warm the instant the member reaches STARTED" guarantee the
engine never promised; this PR removes that false assumption without weakening
a single original assertion.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Single file, test-only:
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/metrics/MetricsApiTest.java`
(+118/-7 against `dev`, confirmed via `git diff dev...HEAD --stat` on the
mirror).
Before (`dev`):
```java
given().get("http://localhost:8080"; + RestConstant.REST_URL_METRICS)
.then()
.statusCode(200)
.body(containsString("process_start_time_seconds"))
.body(containsString("engine_state_store_local_owned_entries"))
.body(containsString("engine_state_store_checkpoint_monitor_jobs"));
```
After (`MetricsApiTest.java:103-107`):
```java
Awaitility.await()
.pollDelay(0, TimeUnit.SECONDS)
.pollInterval(1, TimeUnit.SECONDS)
.atMost(READY_TIMEOUT_SECONDS, TimeUnit.SECONDS)
.untilAsserted(MetricsApiTest::assertMetricsExposed);
```
**Root-cause diagnosis, re-verified against source at this head, not taken
on faith:**
- `SeaTunnelServer.init()` calls `startMaster()` and then starts Jetty
synchronously later in the same method (`SeaTunnelServer.java:188-191` →
`jettyService.createJettyServer()`), so the HTTP listener is accepting
connections by the time `@BeforeAll`'s `createHazelcastInstance()` returns,
while `CoordinatorService` activation is asynchronous.
- `SeaTunnelServer.getCoordinatorService()` (`SeaTunnelServer.java:286-318`)
retries 3×500ms internally and, if still not active, throws
`SeaTunnelEngineRetryableException("Can not get coordinator service from an
active master node.")` (line 313-314).
- `JobMetricExports.collect()` guards with `if (isMaster() &&
isCoordinatorReady())`, where `isCoordinatorReady()` is a non-blocking
`getServer().isCoordinatorActive()` check — a genuine check-then-act window
against the subsequent `getCoordinatorService()` call.
- `MetricsServlet.doGet()` drives `TextFormat.writeFormat(contentType,
stringWriter, collectorRegistry.metricFamilySamples())`, iterating every
collector in one pass with no per-collector isolation, so one throwing
collector aborts the whole scrape.
- `ExceptionHandlingFilter.doFilter()` catches any escaping `Exception` and,
at `handleException()`, sets
`errorResponse.setMessage(ExceptionUtils.getStackTrace(e))` — the full stack
trace, confirming the 500 body genuinely carries diagnosable information, which
is what makes attaching it to the assertion message worthwhile.
This is a real, reachable race on the normal CI path (the test runs on every
PR), not a CI artifact, and the fix targets it directly rather than papering
over the symptom.
**Is this "fix the race" or "loosen the assertion"?** Line-by-line against
the pre-PR single-shot assertion: same status-code expectation, same three
`contains` checks, same strings, re-evaluated on *every* poll iteration, not
once. No threshold widened, no substring dropped, no `Thread.sleep`-and-hope. A
server still returning 500 (or missing a metric family) after 60s still fails
the test — `untilAsserted` re-throws a `ConditionTimeoutException` carrying the
last `AssertionError`'s message, so a genuinely broken endpoint does not pass.
## 1.2 Compatibility Impact
**Fully compatible.** Test-only change; no production code, config option,
default value, protocol, or serialization format is touched.
## 1.3 Performance / Side-Effect Analysis
- Healthy path: `pollDelay(0, TimeUnit.SECONDS)` makes the first request
fire immediately. I checked this claim against the actual `awaitility-4.2.0`
sources (not the changelog text): `ConditionFactory.pollInterval(long,
TimeUnit)` calls `definePollDelay(pollDelay, fixedPollInterval)`, which — when
`pollDelay` was never explicitly set — defaults it to the poll interval for a
`FixedPollInterval`. So without the explicit `pollDelay(0, ...)` line, the
first GET really would be pushed out by ~1s; the line is load-bearing, not
decorative.
- Stalled-connection path, one piece of evidence not previously on this
thread: `ConditionAwaiter.await()` runs each poll on a
`Executors.newSingleThreadExecutor(...)`-backed executor
(`InternalExecutorServiceFactory.create`), and that factory uses the JDK
default `ThreadFactory` — i.e. **non-daemon** threads. The awaiting thread does
correctly bound the *overall* wait via
`getUninterruptibly(currentConditionEvaluation, maxWaitTimeForThisCondition)`,
and on timeout calls `currentConditionEvaluation.cancel(true)` /
`shutdownNow()`. But a plain blocking socket read (Apache HttpClient's
synchronous transport, as used here via RestAssured) does not respond to
`Thread.interrupt()`. So absent `REQUEST_TIMEOUT_MILLIS`, a request that truly
hangs (TCP connected, server never responds, no RST) would leave a
**non-daemon** evaluation thread blocked on the socket read past `atMost`'s
expiry — not just "wastes the retry budget" as the in-code comment says, but a
thread that can outlive th
e test method and, in the worst case, keep the Surefire JVM fork from exiting
cleanly. `REQUEST_TIMEOUT_MILLIS = 5_000` closes this precisely, and I'd call
it a more concrete justification for the constant than the current Javadoc
states, not a gap in the PR.
- Broken-endpoint path: failure detection moves from instant to bounded at
60s — well inside the `unit-test` job's overall timeout, and only paid when the
test was going to fail anyway.
- Resource release: `@AfterAll` still calls `instance.shutdown()`
unconditionally; no new executor/thread/connection pool is introduced by the
test code itself.
- Log volume: `truncateForLogging` bounds the missing-metric-path message to
4KB across up to 60 retried assertions; the 500 path deliberately keeps the
full body (that's where the stack trace lives).
## 1.4 Error Handling and Logging
No new formal issue at this head — every issue raised across 16 prior rounds
(blanket `ignoreExceptions()` retrying unrelated `Throwable`s, missing
per-request timeout, implicit 1s poll delay, unbounded body logging) is
resolved in the current implementation. One item remains open, carried forward
unchanged and correctly scoped out of this PR:
**Issue 1: Hardcoded `localhost:8080`**
- **Location:** `MetricsApiTest.java:46-47` (URL constant), `:80` (port set
in `before()`).
- **Problem description:** the metrics URL is built against a fixed port
rather than an actually-bound or ephemeral one. Pre-existing on `dev`, not
introduced by this diff.
- **Potential risk:** if another process on a CI runner holds 8080,
`server.start()` fails or the test could hit a foreign server. With the
timeout/diagnostic hardening now in place, a collision at least fails fast
(within the 5s per-request bound) with a connection error instead of an opaque
60s timeout.
- **Best improvement:** follow-up PR moving this test (and similarly-shaped
REST tests) onto `HttpConfig`'s dynamic-port mode.
- **Severity:** Low, non-blocking.
- **Raised by another reviewer:** Yes — raised independently by the author's
own self-review and by @SEZ9 across multiple earlier rounds; +1, still agree
it's correctly out of scope for a targeted stability fix.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Import grouping/ordering follows project convention; the now-unused
`org.hamcrest.Matchers.containsString` static import is correctly removed.
Every new constant carries Javadoc explaining its purpose and constraints, and
the new private methods are documented. The inline rationale comment above the
poll explains the race and the design choices (`pollDelay(0)`,
rethrow-as-`AssertionError` instead of `ignoreExceptions()`) — "why," not
"what."
## 2.2 Test Coverage and Test Stability
Coverage unchanged in scope: same endpoint, same 200 expectation, same three
metric-family checks, retained verbatim inside the polled assertion.
**Mandatory flaky-test analysis, re-derived at this head:**
- No hard sleep anywhere — `Awaitility.await()...untilAsserted(...)` is a
genuine condition-driven poll that re-issues the real HTTP call and re-checks
all three assertions every iteration, not a fixed-duration sleep followed by a
single assertion.
- Non-deterministic-timing dependency removed, not added — the pre-change
test's single immediate GET was exactly the "assert before the event has
happened" anti-pattern; the poll replaces it with a bounded, re-checked
condition.
- Transport-exception blind spot closed correctly and narrowly:
`assertMetricsExposed()` wraps only the HTTP call itself in `try { ... } catch
(Exception e) { throw new AssertionError(...) }` (`:129-146`) — a real bug
elsewhere in the helper (e.g. in `assertContains`) still surfaces as its own
`AssertionError` immediately via the normal JUnit assertion path, not silently
retried behind a broad `ignoreExceptions()`. One narrow, non-blocking residual:
`response.getBody().asString()` (`:148`) sits *outside* the try/catch, so if
reading an already-received response body ever threw a checked or unchecked
exception, it would propagate un-converted and abort the poll early via
Awaitility's `catch (Throwable) -> CheckedExceptionRethrower.safeRethrow`. In
practice RestAssured buffers the body eagerly during `.get(...)`, so this is a
theoretical edge, not a reproducible one — noting it for completeness rather
than as a blocking finding.
- No resource leak: `@AfterAll` unconditionally shuts down the instance; no
new executor/thread is introduced by the test's own code (see 1.3 for why the
*library's* executor threading model still matters here).
- No log-flooding risk: truncation on the missing-metric path bounds message
size across up to 60 retried assertions.
- Live evidence at this exact head (`be6b805`): fork CI's `unit-test (8,
ubuntu-latest)`, `unit-test (11, ubuntu-latest)`, `unit-test (8,
windows-latest)`, `unit-test (11, windows-latest)` — the only lanes that
execute this test — and the apache-side required `Build` check are all green as
of this pass (`gh pr checks`: `Build pass, 53m32s`).
**Stability rating: Stable.** No flaky-test anti-pattern remains in the
current implementation. The one open item (Issue 1, hardcoded port) is a
pre-existing, low-risk, non-blocking carryover, not something this diff
introduces or worsens.
## 2.3 Documentation Updates
Not applicable — no user-facing behavior, config, or API changed, so no
`docs/en` / `docs/zh` update is required. In-code documentation is present and
accurate.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Precise fix.** Targets the actual defect (an unstated timing assumption)
instead of hardening every collector's check-then-act readiness guard on the
production side, which would be a much larger change for a CI-stability goal.
The production-side gap is correctly split into a tracked follow-up (#11846)
instead of being conflated with this test fix.
## 3.2 Maintainability
Good. The response-body-attached failure message makes the next genuine
failure self-diagnosing from CI logs alone — the property the old test lacked,
and the actual reason these failures went undiagnosed across five blocked PRs.
The rationale comment should deter a future "simplify this back to one GET"
regression.
## 3.3 Extensibility
The `assertMetricsExposed` / `assertContains` split makes adding another
metric-family assertion a one-line change; the same await-with-diagnostic shape
could be lifted into a shared test utility if other REST-layer tests hit the
same coordinator-wiring window.
## 3.4 Historical-Version Compatibility
Not applicable in the strict sense (no shipped
artifact/protocol/serialization change). Worth noting: unlike the `dev`
version, this test no longer silently depends on today's exact startup timing,
so it stays correct if the coordinator-wiring window changes shape in a future
engine version.
# 4. Issue Summary
| No. | Issue | Location | Severity |
|-----|-------|----------|----------|
| 1 | Hardcoded `localhost:8080` (pre-existing on `dev`, kept by this PR) |
`MetricsApiTest.java:46-47,80` | Low, non-blocking |
No blocking issues found in the code at this head.
# 5. Merge Recommendation
### Conclusion: Ready to merge
1. **Blockers — must be fixed:** none. `reviewDecision` on the PR is already
`APPROVED` (@davidzollo and @nzw921rx), the required `Build` check on this
exact head (`be6b805`) is green (`pass`, 53m32s — a full run, not a
short-circuited sanity pass), and the four `unit-test` lanes that actually
execute `MetricsApiTest` all report success. My own 10:28 UTC comment on this
thread, reporting `Build` at `failure` due to unrelated `all-connectors-it-1` /
`jdbc-connectors-it-ddl` IT shards, is now stale — that run has since been
superseded by a green one on the same head, so the "sync with dev and rerun"
recommendation in that comment no longer applies.
2. **Recommended fixes — non-blocking:**
- Issue 1 (hardcoded port): small follow-up PR moving this and
similarly-shaped REST tests onto dynamic-port mode.
Overall assessment: the diagnosis is backed by source on both the production
race and the response-body claim, re-verified independently at this head
against the engine sources and against the actual `awaitility-4.2.0` library
sources (not just prior review text). The fix removes a real timing assumption
without weakening a single original assertion, the diagnostic value (full
response body on failure) is the part with lasting worth, and CI now confirms
the exact previously-flaking lanes are green at this head. I don't see anything
left to block merge.
--
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]
