DanielLeens commented on PR #11641:
URL: https://github.com/apache/seatunnel/pull/11641#issuecomment-5340845595
A note up front: this is my own PR, so this is a self-review and cannot
count as an independent approval — GitHub blocks self-review, so this is posted
as a plain comment. There is no new commit since @davidzollo's `APPROVED`
review on this exact head (`be6b805`), so this pass is not a re-review of new
code. What changed is a CI fact: my own last comment on this thread said the
required `Build` check was `cancelled` and that I'd triggered a rerun to get a
real signal. That rerun has since completed, and the picture is different from
what I described, so I'm correcting it here rather than leaving stale guidance
on the thread. I also independently re-derived every source-level claim below
from the current head rather than carrying anything over from the previous 16
rounds.
# 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, blocking merges that had nothing to do with
metrics.
- **Fix approach:** replace the single, immediate assertion with a bounded
(60s, 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 so a
genuine failure is diagnosable from CI logs instead of a bare status code.
- **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, and this PR removes that false assumption without
weakening any of the original assertions.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Single file, test-only change:
`seatunnel-engine/seatunnel-engine-server/src/test/java/org/apache/seatunnel/engine/server/metrics/MetricsApiTest.java`
(verified via `gh pr diff --name-only` — no other file is touched despite the
branch being 28 commits behind `dev`).
Before (on `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 (current head, `MetricsApiTest.java:103-107`):
```java
Awaitility.await()
.pollDelay(0, TimeUnit.SECONDS)
.pollInterval(1, TimeUnit.SECONDS)
.atMost(READY_TIMEOUT_SECONDS, TimeUnit.SECONDS)
.untilAsserted(MetricsApiTest::assertMetricsExposed);
```
with `assertMetricsExposed()` (`:127-156`) wrapping the GET in a 5s
connect/socket timeout, rethrowing transport failures as `AssertionError`
(`:142-146`), and asserting status + all three original metric families with a
truncated-body failure message (`:158-178`).
**Root-cause diagnosis, re-verified against source today rather than trusted
from the PR description:**
- `SeaTunnelServer.init()` starts the Jetty listener synchronously:
`jettyService.createJettyServer()` is called directly inside `init()`
(`SeaTunnelServer.java:187-189`), and `JettyService.createJettyServer()` calls
`server.start()` synchronously (`JettyService.java:260`). So the HTTP listener
is already accepting connections by the time `@BeforeAll` returns.
- `CoordinatorService` becomes active on a separate, asynchronous path.
`SeaTunnelServer.getCoordinatorService()` (`SeaTunnelServer.java:286-318`)
throws `SeaTunnelEngineRetryableException` if the coordinator isn't active yet
even after its own internal wait/retry.
- `ExceptionHandlingFilter.doFilter()` catches any exception escaping the
servlet chain and maps it to HTTP 500, and — this is the detail I actually got
wrong in an earlier round and corrected on Aug 13 — the body is not just
`e.getMessage()`, it's the full stack trace:
`errorResponse.setMessage(ExceptionUtils.getStackTrace(e))`
(`ExceptionHandlingFilter.java:66`). So a collector that touches not-yet-wired
coordinator state during that narrow window produces a 500 whose body carries
the originating stack trace — which the pre-change test discarded entirely,
since Rest Assured's `statusCode(200)` only reports the code.
Given that, the fix is correctly targeted: it doesn't paper over the race
with a fixed sleep, it re-checks the real endpoint on every iteration (a
genuinely broken endpoint still fails, just after up to 60s instead of
instantly), and it makes the eventual failure carry the diagnostic that was
previously thrown away.
**Key Findings**
- The normal test-execution path reaches the changed logic directly — this
is the entire body of `metricsApiTest()`, not a defensive/recovery branch.
- The scenario covered is exactly the observed CI flake: a scrape landing in
the STARTED-but-coordinator-not-yet-active window.
- This is a precise fix, not a workaround: it removes an incorrect timing
assumption rather than hiding the symptom, and the actual production-side
readiness gap (a real Prometheus scraper hitting the same window) is tracked
separately rather than silently dropped — I confirmed
`https://github.com/apache/seatunnel/issues/11846` exists, is open, and its
title matches the diagnosed gap.
- No remaining code-level blind spot from the last 16 rounds of discussion:
the two issues the author's own self-review originally flagged (no retry on
non-`AssertionError` transport failures; implicit 1s first-poll delay) are both
resolved at this head, and I re-verified both mechanisms directly in the
current file rather than taking the changelog at its word.
## 1.2 Compatibility Impact
**Fully compatible.** Test-only change; no production code, configuration
option, default value, protocol, or serialization format is touched. `gh pr
diff --name-only` confirms the diff is confined to this one test file.
## 1.3 Performance / Side-Effect Analysis
- Healthy path: `pollDelay(0, TimeUnit.SECONDS)` (`:104`) makes the first
request fire immediately — Awaitility's fixed-interval strategy otherwise
defaults the poll delay to the poll interval when not set explicitly, so this
line is load-bearing, not decorative.
- Broken-endpoint path: failure detection moves from instant to bounded at
60s, which only costs time on a run that was going to fail anyway, and is well
inside the `unit-test` job's overall timeout.
- Stalled-connection path: the 5s `http.connection.timeout` /
`http.socket.timeout` (`:135-140`) bounds each individual attempt, so one hung
TCP read can't silently consume the entire 60s budget on a single blocked
evaluation.
- Resource release: `@AfterAll` still calls `instance.shutdown()`
unconditionally (`:110-115`); the poll itself introduces no new resource (no
executor, no background thread — `untilAsserted` blocks the calling test thread
synchronously).
- Log volume: the missing-metric path truncates the body via
`truncateForLogging` (`:170-178`) so a large Prometheus exposition payload
doesn't flood CI output across up to 60 retried assertions; the 500 path
deliberately keeps the body untruncated at `:152` because that's where the
stack-trace diagnostic actually lives.
## 1.4 Error Handling and Logging
No new formal issue at this head. All issues raised across the review
history (blanket `ignoreExceptions()` retrying unrelated `Throwable`s, no
per-request timeout, implicit poll delay, unbounded body logging) are resolved
in the current implementation, and I re-verified each fix directly against the
file rather than trusting the changelog. One item remains open, carried forward
unchanged from earlier rounds:
**Issue 1: Hardcoded `localhost:8080`**
- **Location:** `MetricsApiTest.java:46-47` (URL constant) and `:80` (port
set in `before()`).
- **Problem description:** the metrics URL is built against a fixed port
rather than one derived from the actually-bound port or an ephemeral port.
- **Potential risk:** if anything else on a CI runner holds 8080,
`server.start()` fails or the test could hit a foreign server. This is
pre-existing on `dev` and not introduced or worsened by this diff; with the
timeout/diagnostic fixes now in place, a collision at least fails with a
connection error inside the 5s per-request bound instead of an opaque 60s
timeout.
- **Best improvement:** a follow-up PR moving this test onto `HttpConfig`'s
dynamic-port mode.
- **Severity:** Low
- **Raised by another reviewer:** Yes — raised independently across multiple
earlier rounds (the author's own self-review and @SEZ9); not resolved by design
since it's out of scope for a targeted stability fix, and I agree with that
scoping decision.
# 2. Code Quality Assessment
## 2.1 Coding Standards
Import grouping/ordering follows project convention; the now-unused static
import (`org.hamcrest.Matchers.containsString`) is correctly removed. Every new
constant (`METRICS_URL`, `READY_TIMEOUT_SECONDS`, `REQUEST_TIMEOUT_MILLIS`,
`MAX_LOGGED_BODY_CHARS`) carries a Javadoc explaining its purpose and
constraints (`:49-71`), and both new private methods (`assertMetricsExposed`,
`assertContains`, `truncateForLogging`) are documented or short enough to be
self-evident. The inline rationale comment above the poll (`:87-102`) explains
the race, why `pollDelay(0)` is explicit, and why transport failures are
rethrown instead of swallowed — this is exactly the kind of "why, not what"
comment this protocol asks for on non-trivial test logic.
## 2.2 Test Coverage and Test Stability
Coverage is unchanged in scope: same endpoint, same 200 expectation, same
three metric-family checks, all retained verbatim inside the polled assertion
(`:153-155`). What changed is when the test gives up and what a failure reports
— the test is not weakened by this PR.
**Mandatory test-stability analysis (Section 5.10.2), re-derived from the
current file:**
- No hard wait: the mechanism is a genuine condition-driven poll
(`Awaitility.await()...untilAsserted(...)`, `:103-107`) that re-evaluates the
real HTTP call and all three assertions every iteration, not a fixed-duration
sleep followed by one assertion.
- Non-deterministic-timing dependency removed, not added: the pre-change
test's single immediate GET is exactly the "assert before the event has
actually happened" anti-pattern this section warns about; the poll replaces
that assumption with a bounded, re-checked condition.
- Transport-exception blind spot closed: `assertMetricsExposed()` catches
`Exception` from the HTTP call and rethrows as `AssertionError` with the
original exception as cause (`:142-146`), so `untilAsserted` (which by contract
only retries `AssertionError`) now retries connection-level transients under
the same 60s bound instead of aborting the poll on first occurrence — this was
the exact gap the author's own self-review flagged in the first round, and it
is closed here without reintroducing the earlier `ignoreExceptions()` version's
problem of also swallowing genuinely non-transient exceptions (a real bug in
the helper would still surface immediately as itself, not get silently retried
for 60s, since only the explicit HTTP-call `try/catch` is narrowed to
`Exception`, not the assertions below it).
- No resource leak: `@AfterAll` unconditionally shuts down the instance
(`:110-115`); no new executor, thread, or connection pool is introduced.
- No log-flooding risk: truncation on the missing-metric path (`:170-178`)
bounds the message size across up to 60 retried assertions.
- Hardcoded port remains (Issue 1 above) — a real but pre-existing,
Low-severity, non-blocking anti-pattern, not newly introduced.
- Live evidence at this exact head (`be6b805`, fork run `32092429934`):
`unit-test (8, ubuntu-latest)`, `unit-test (11, ubuntu-latest)`, `unit-test (8,
windows-latest)`, and `unit-test (11, windows-latest)` — the only lanes that
execute this test — all report `conclusion: success`.
**Stability rating: Stable.** No remaining flaky-test anti-pattern was found
in the current implementation; the one open item (Issue 1, hardcoded port) is a
pre-existing, low-risk, non-blocking carryover rather than something this diff
introduces or worsens, and it does not warrant a "Risk present" rating on its
own given the timeout/diagnostic hardening already in place around it.
## 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 (constant
Javadoc, method Javadoc, rationale comment) is present and, as far as I can
re-verify today, accurate.
# 3. Architectural Soundness
## 3.1 Elegance of the Solution
**Precise fix.** It targets the actual defect (an unstated timing
assumption) rather than 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 out into a
tracked follow-up issue 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 for months. 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. If other REST-layer tests in this
module race the same coordinator-wiring window, this shape is small enough to
lift into a shared test utility later — premature to do so for one test.
## 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 after fixes
1. **Blockers — must be fixed**
- Not a code blocker. The required `Build` check on this head (`be6b805`)
currently reports `failure`, and that needs to turn green before this can
merge, but it is not evidence of a defect in this diff. Diagnosis below.
2. **Recommended fixes — non-blocking**
- Issue 1 (hardcoded port): worth a small follow-up PR moving this and
similarly-shaped REST tests onto dynamic-port mode. Predates this diff and
isn't this PR's job to fix.
**CI diagnosis (correcting my own Aug 18 03:33 UTC comment on this
thread):** I previously said the apache-side `Build` check showed `cancelled`
and that I'd triggered a rerun to get a real signal. That rerun (fork run
`32092429934`) has since completed with `conclusion: failure`, so the situation
has changed and I want to be precise about what that failure actually is rather
than leave the old "wait for the rerun" guidance standing:
- The four `unit-test` jobs — `unit-test (8, ubuntu-latest)`, `unit-test
(11, ubuntu-latest)`, `unit-test (8, windows-latest)`, `unit-test (11,
windows-latest)` — the only jobs that execute `MetricsApiTest` — all report
`conclusion: success`.
- The jobs that actually failed/cancelled are `all-connectors-it-1 (8,
ubuntu-latest)` (failure), `all-connectors-it-1 (11, ubuntu-latest)`
(cancelled), `paimon-connector-it (8, ubuntu-latest)` (cancelled), and
`jdbc-connectors-it-ddl (11, ubuntu-latest)` (failure) — all connector-v2 IT
suites in modules this PR does not touch (the diff is one file under
`seatunnel-engine-server/src/test`).
- This branch is `behind_by=28` against `dev`, and I found concrete evidence
the `all-connectors-it-1` failure is a known, already-fixed-on-`dev` issue that
simply postdates this branch's last `dev` merge: `dev` commit `449db4ef`
("[Test][E2E] Retry Couchbase container startup and capture server logs")
landed at `2026-08-18T05:44:39Z`, about three hours *after* this fork's CI run
started (`2026-08-18T02:36:08Z`) — the Couchbase-container-bootstrap flake it
fixes is exactly the kind of failure that kills `all-connectors-it-1` shards.
Separately, `dev` commit `5b358f8b` ("[Fix][CI] Add fail-fast: false to
backend.yml matrix jobs") landed today (`2026-08-19T07:01:00Z`) and explains
why sibling jobs in the same matrix show `cancelled` rather than their own real
result once one job in the group fails. I did not find a specific dev-side fix
for the `jdbc-connectors-it-ddl` failure, so I can't claim the same direct
evidence there, but it's in an unrelated module either way.
- So: please sync with the latest `dev` and rerun CI. Given the evidence
above, that's a real fix for at least the `all-connectors-it-1`/cancellation
pattern, not just a generic "try again." If `jdbc-connectors-it-ddl` still
fails after the sync, that would need its own triage, but it would not be
evidence against this PR's diff.
**Response to the existing review state:** @davidzollo's `APPROVED` review
on this exact head independently re-verified the `awaitility-4.2.0` internals
(poll-delay default, `AssertionCondition` message handling) against bytecode. I
agree with that conclusion — I re-derived the same claims today from the source
files directly (`SeaTunnelServer.java`, `JettyService.java`,
`ExceptionHandlingFilter.java`) rather than re-trusting the earlier rounds, and
they hold up. `reviewDecision` on the PR already shows `APPROVED`; the only
thing standing between this and mergeable is the CI gate described above, not
any remaining code-side concern from my side.
Overall assessment: the diagnosis is backed by source on both the production
race and the response-body claim, 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 live CI already
confirms the exact previously-flaking lanes are green at this head. Once this
branch is synced to current `dev` and the `Build` check comes back green on the
actually-relevant lanes, 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]
