DanielLeens commented on PR #11626: URL: https://github.com/apache/seatunnel/pull/11626#issuecomment-5459107922
Thanks for the deep follow-up, @SEZ9 — I rechecked the current head (`0b18d9d3`) against each of your eight points before replying, rather than taking them at face value. All eight hold up against the source: - Issue 1 (nested `/tmp` configs invisible to the baseline): confirmed, `listTemporaryConfigs()` is `find /tmp -maxdepth 1 -type f ...` (`SeaTunnelContainer.java:279-283`) — anything copied under a subdirectory (e.g. `/tmp/copy/copy_transform.conf`) is genuinely outside both `deleteAddedArtifacts` and `assertArtifactsRestored`. - Issue 2 (non-atomic / non-invalidated baseline fields): confirmed, `prepareForTestClass()` (`:244-249`) assigns `connectorJarsBeforeTest` then `temporaryConfigsBeforeTest` with no local-capture-then-commit and no reset on partial failure. - Issue 3 (classloader-cache staleness on same-named jar restore): I don't think this is speculative — `classloader-cache-mode` is a real Zeta config option (`ServerConfigOptions.java`), and the restore logic here is purely name-based (`:263-277`), so the risk you describe is a legitimate consequence of combining the two, even though neither `FakeIT` nor `FakeSqlConfIT` currently swaps same-named jars with different content. - Issue 4 (baseline missing subdirs / `$SEATUNNEL_HOME/lib`): confirmed, same `-maxdepth 1` limitation applies to `listConnectorJars()` (`:273-277`). - Issues 5/6/8 (`assertNoRunningJobs` has no connect/socket timeout and is single-shot, no retry): confirmed, `HttpClients.createDefault()` at `:343` builds no `RequestConfig`, and the call is a bare one-shot GET. I'd treat 5, 6, and 8 as the same underlying gap (missing `RequestConfig` + missing `Awaitility`-style poll) rather than three separate fixes. - Issue 7 (`assertVolumeEmpty` conflates exec failure with non-empty, reports stdout not stderr): confirmed, `:331-334` throws the same message and payload (`result.getStdout()`) for both `exitCode != 0` and a non-empty listing, so an exec/permission failure surfaces as a misleading empty "volume is not empty:" error. Where I land on scope: my prior approval focused on the class-lifecycle mechanics (acquire/release, semaphore serialization, restart-on-failure) rather than the robustness of these individual probe helpers, and none of your eight points were covered there — that's a fair gap in my review, not a disagreement with your findings. Within the two adopters landing in this PR (`FakeIT`, `FakeSqlConfIT` — flat single-file fake configs, no jar-content churn), none of these currently produce an observable failure, which matches your own framing that the mechanics are sound for the fake module today. But they are real latent gaps in shared infrastructure that any future class opting into `@ReuseTestContainers` would inherit silently, so I agree they're worth resolving (or at minimum tracked in a follow-up issue/PR) before reuse is adopted beyond the current two classes, rather than being treated as optional polish. I'm not asking for a formal re-review cycle on this thread right now since there's no new commit yet — once a fix (or a decision to defer via a tracked follow-up issue) lands, I'll re-review the updated head in full. -- 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]
