DanielLeens commented on PR #11814:
URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-5652845546
Thanks for the checklist — fair ask given how long this thread has run. Head
is still `af713e463f`, no new commit since my last comment, so I re-read the
actual current source for each item below rather than pointing back at old
commit messages.
**1. F1 (wait strategy in `FakeSourceToConsoleWithEventReportIT`).** I want
to flag something before answering: you yourself marked this "Resolved previous
findings" in your 2026-08-30 review (`f83cb22736e4`), and nothing in
`FakeSourceToConsoleWithEventReportIT.java` or its parent container class has
changed since then (byte-identical, per both of our confirmations this week). I
re-traced the actual call chain again today rather than relying on that prior
conclusion: `SeaTunnelEngineContainer.startUp()` calls `super.startUp()`, which
(`SeaTunnelContainer.java:98-101`) calls `createSeaTunnelServer()` ->
`createSeaTunnelServer(NETWORK)`. Inside that method,
`executeExtraCommands(server)` is invoked at line 146, and `server.start()` is
only called afterward, at line 148. So the custom `LogMessageWaitStrategy` this
test sets inside its `executeExtraCommands` override
(`FakeSourceToConsoleWithEventReportIT.java:106-117`) is applied to the
container's configuration *before* `start()` r
uns, not after — it does reliably gate readiness on this path, it's not just
"happens to pass." (The ordering you're describing — `start()` before
`executeExtraCommands()` — does exist elsewhere in this file, in
`createSeaTunnelContainerWithFakeSourceAndInMemorySink` at
`SeaTunnelContainer.java:197/199`, but that overload isn't on this test's call
path; it goes through `startUp()`, not that method.) If you're seeing a
different call path than this, I'd genuinely like the specific trace, since
what I have here says this was correctly fixed before your 08-30 round and
hasn't regressed.
**2. F2/F3 (`close()` shutdown hardening).** Both fixed, confirmed against
current `JobEventHttpReportHandler.java`:
- The ringbuffer/local-buffer flush is now gated on `schedulerTerminated &&
!interrupted` (`:237-251`); when the scheduler doesn't terminate, or the wait
was interrupted, the `else` branch just logs a warning and skips the flush
entirely (`:250`) — no more concurrent flush racing an
interrupted-but-still-running task.
- The two flush calls are each wrapped in `catch (Exception e)` (`:241`,
`:246`), not the narrow `HazelcastInstanceNotActiveException`/`IOException`
pair.
- `httpClient.dispatcher().cancelAll()` runs before
`httpClient.connectionPool().evictAll()` in the `finally` block (`:253-254`),
so in-flight connections are cancelled before eviction rather than being left
parked for the keep-alive window.
**3. F4/F6 (Kotlin/okio version split).** Fixed. `known-dependencies.txt`
now lists `kotlin-stdlib`, `kotlin-stdlib-common`, `kotlin-stdlib-jdk7`,
`kotlin-stdlib-jdk8` all at `1.9.10` (no more 1.8.21), and
`seatunnel-dist/release-docs/LICENSE` matches it line for line.
`seatunnel-engine-server/pom.xml` now imports `kotlin-bom:1.9.10` in
`dependencyManagement` (`:33-34`), which is what pins the graph to one version
instead of leaving it to Maven nearest-wins.
**4. F5 (`testRetryAfterHttpFailure` hygiene).** Fixed.
`JobEventHttpReportHandlerTest.java:208-209` now uses
`retryServer.takeRequest(10, TimeUnit.SECONDS)` for both requests (bounded,
won't hang the fork), and the two `Buffer`s are read inside try-with-resources
(`:212-213`).
**5. F7 (redirect following).** Fixed. `createHttpClient()` builds with
`.followRedirects(false).followSslRedirects(false)`
(`JobEventHttpReportHandler.java:273-274`), so a 3xx now surfaces as a non-2xx
through the existing retry/log path instead of forwarding configured headers to
a redirect target.
**6. F8 (Content-Type charset parity).** Fixed. `testReportEvent` asserts
`"application/json; charset=utf-8"` against `request.getHeader("Content-Type")`
(`JobEventHttpReportHandlerTest.java:175`), so the migration's header behavior
is locked in by the test, not just asserted in a review comment.
All eight are in on `af713e463f`, verified against the current files just
now rather than carried forward from memory. CI on this exact head is green on
the two jobs that exercise this diff (`engine-v2-it` 8 and 11 on
ubuntu-latest), as I reported after the last push; the only failures on that
run are the `minio/minio` registry-pull issue hitting the whole CI fleet in
modules this PR doesn't touch. Nothing outstanding from my side.
--
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]
