DanielLeens commented on PR #10937:
URL: https://github.com/apache/seatunnel/pull/10937#issuecomment-4528213676
Thanks for the update. I re-reviewed the latest head from scratch and traced
the changed JDBC E2E paths again.
# What This PR Fixes
- User pain: several JDBC E2Es were still relying on weak readiness signals,
moving wall-clock timestamps, or timing guesses, so CI could fail because the
environment was not actually ready yet.
- Fix approach: the new commits add deterministic date/time test data for
IRIS/Xugu, add real JDBC readiness checks for MySQL, and replace the old fixed
retry loop in Presto/Trino with condition-based waiting.
- One-line summary: most of the latest stabilizations are in the right
direction, but the new Presto readiness gate is still too weak for the actual
DDL path and the PR is still red because of that.
Simple example:
- The latest `JdbcPrestoIT` waits until `SELECT count(*) FROM
system.runtime.nodes WHERE state = active` succeeds.
- But in CI the very next `CREATE TABLE` still fails with `No nodes
available to run query`, so the probe is proving that the coordinator can
answer a system query, not that the test can already schedule the first real
write-side statement.
# Runtime Chain Rechecked
```text
Presto test bootstrap
-> JdbcPrestoIT.initializeJdbcConnection(...)
[seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcPrestoIT.java:91-134]
-> open JDBC connection
-> Awaitility waits on `SELECT count(*) FROM system.runtime.nodes
WHERE state = active`
First real DDL path
-> JdbcPrestoIT.createNeededTables() [JdbcPrestoIT.java:136-149]
-> statement.execute(createSource)
-> CI failure: `No nodes available to run query`
Build evidence
-> fork run `nzw921rx/seatunnel#26355151274`
-> job `updated-modules-integration-test-part-8`
-> `JdbcPrestoIT` errors at create-table time
```
# Key Findings
- The deterministic timestamp changes in `JdbcIrisIT` / `JdbcXuguIT` are
good and remove unnecessary wall-clock variance.
- The new MySQL / Metalake readiness gates are also stronger than the
earlier fixed sleeps.
- `JdbcTrinoIT` passed on the latest CI run, so the new Trino wait looks
acceptable.
- The remaining blocker is now very specific: `JdbcPrestoIT` still proves
readiness with the wrong signal.
# Findings
Issue 1: `JdbcPrestoIT` still marks the server ready too early, and the
first actual DDL path is still failing in CI
- Location:
-
`seatunnel-e2e/seatunnel-connector-v2-e2e/connector-jdbc-e2e/connector-jdbc-e2e-part-7/src/test/java/org/apache/seatunnel/connectors/seatunnel/jdbc/JdbcPrestoIT.java:110-149`
- CI log: `nzw921rx/seatunnel` run `26355151274`, job
`updated-modules-integration-test-part-8`
- Problem description:
The new wait checks whether `system.runtime.nodes` reports an active node,
but that does not guarantee the first real `CREATE TABLE` can already be
scheduled. The current run still fails immediately afterward with
`SeaTunnelRuntimeException: Fail to create table` caused by
`java.sql.SQLException: No nodes available to run query`.
- Potential risk:
This is still a merge blocker because the main purpose of the PR is JDBC
E2E stabilization, and one of the changed E2Es is still red on the latest head.
- Best improvement suggestion:
- Option A: gate readiness on the first real DDL capability, for example
retry `CREATE TABLE` (or an equivalent write-side probe) until it succeeds
instead of using `system.runtime.nodes` as the only signal.
- Option B: if you want to keep a pre-check, wait on a signal that proves
the scheduler can run normal queries, not just that the coordinator can answer
a system table query.
- Severity: High
- Already raised by others: No
# Test Stability Assessment
- Rating: High risk
- Basis:
- the latest CI run still has a deterministic `JdbcPrestoIT` failure on
the exact path touched by this PR
- the new readiness assertion is still weaker than the real execution
condition it is supposed to protect
# Merge Decision
### Conclusion: can merge after fixes
1. Blocking items
- Issue 1: make the `JdbcPrestoIT` readiness gate prove the actual DDL path
is ready; the current signal is still too weak and the latest Build is red
because of it.
2. Suggested follow-up
- The other latest changes (deterministic JDBC test data, MySQL readiness,
Metalake HTTP readiness) look good to me. I would keep those as-is and narrow
the next fix to the Presto path.
Overall, I still agree with the direction of this PR. But the current head
is not merge-ready yet, because `JdbcPrestoIT` is still failing on the latest
Build for the exact stabilization path this PR is trying to harden.
--
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]
