DanielLeens commented on PR #11503: URL: https://github.com/apache/seatunnel/pull/11503#issuecomment-5550972547
## Correction to my prior APPROVED review: found a real (if intermittent) failure inside this PR's own diff The `Build` on this head (`1e0d7ab0e`) finished with 7 failing jobs. I traced all of them rather than assuming they're all the now-familiar unrelated flakes (Maven Central timeouts, the RocketMQ topic-route race, etc.), and most are: `paimon-connector-it` failed on an unrelated privilege/catalog-isolation error in `connector-paimon-e2e` (nothing this PR touches), `transform-v2-it-part-1` failed on `TestFilterRowKindIT` (a row-kind-filter test this PR's transform changes — `FieldRenameTransform`/`TableRenameTransform`/`SQLTransform`/`CalciteTransform` — don't touch at all), and `all-connectors-it-4`/`all-connectors-it-7` are outside this diff's file list entirely. **`doris-connector-it (11)` is different: it failed inside `DorisErrorIT.java`, a file this branch's own commit history modified** (the "avoid brittle Doris error stack assertions" round). Traced the actual failure: - `DorisErrorIT.testDoris` stops the Doris container mid-stream-load and asserts the resulting exception's stderr contains three things: `DorisConnectorErrorCode.STREAM_LOAD_FAILED`'s code, a specific stack frame (`RecordBuffer.checkErrorMessageByStreamLoad`), and (new in this PR) `STREAM_LOAD_FAILED`'s description. - The reported failure is at `DorisErrorIT.java:84` — the second assertion (the stack-frame check). - The actual captured stderr in this run shows the sink retrying and eventually failing with `org.apache.seatunnel.connectors.doris.exception.DorisConnectorException: ErrorCode:[Doris-01], ErrorDescription:[stream load error]`, root-caused by `java.net.UnknownHostException: doris_e2e: Temporary failure in name resolution` — i.e. after `super.container.stop()`, the sink's retry-and-abort path hit a DNS-resolution failure (container gone, hostname no longer resolvable) rather than the specific stream-load-timeout path the test is designed to exercise through `RecordBuffer.checkErrorMessageByStreamLoad`. This is a genuine race in the test's own design — stopping the container doesn't deterministically produce the same failure path every time, and it happens to be inside a hunk this branch itself modified, not `dev` baseline. It's not a bug in the CDC schema-restore feature this PR is actually about, and I can't say from the evidence I have whether this exact race pre-dates this PR's commit or was made more likely to surface by the extra assertion — but either way, it's not something I can wave through as "clearly unrelated CI noise" the way I did for the other 6 jobs, since it's inside the PR's own diff and I don't have a strong signal that a re-run will reliably fix it rather than intermittently repeating. **Correcting my prior formal review**: I'm posting this as `COMMENTED` rather than leaving my earlier `APPROVED` as the last word, since new evidence changed the picture. I'm not marking this a hard blocker requiring a code change from me to diagnose further right now (I don't have enough certainty about the DNS-race mechanics to prescribe a specific fix without guessing), but @nielifeng — worth a look: either the test should tolerate the DNS-failure exception path as an equally-valid outcome of "container stopped mid-load," or the container-stop sequencing needs to guarantee the stream-load-timeout path is what actually fires. A single re-run may well pass (this looks probabilistic, not deterministic), but that wouldn't rule out it recurring on a future CI run. -- 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]
