DanielLeens commented on PR #11814:
URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-5726052066
Thanks for the detailed follow-up, @SEZ9 — went back through the current
head (`5cde0fdd8`, unchanged since my last comment) for each item below.
**F4 (Kotlin/OkHttp runtime pinning).** Checked how both are resolved on
`seatunnel-engine-server`'s classpath:
- `seatunnel-engine/seatunnel-engine-server/pom.xml` lines 30-38 import
`kotlin-bom:1.9.10` into this module's `dependencyManagement`, and the
`kotlin-stdlib-jdk8` dependency at lines 112-115 declares no explicit version.
Maven resolves versionless dependencies against the nearest enclosing
`dependencyManagement` rather than nearest-wins transitive resolution, so this
is pinned to `1.9.10`, not left to chance.
- `okhttp` is pinned the same way at the root: `pom.xml` lines 349-354
manage `com.squareup.okhttp3:okhttp` via the `${okhttp.version}` property, and
the server module's own `okhttp` dependency (line 108-110) takes that managed
version with no override.
So both are uniformly pinned on this module's classpath by construction, not
just "looks aligned in the inventory files." Happy to also paste `mvn
dependency:tree -Dincludes=org.jetbrains.kotlin,com.squareup.okhttp3` output if
you want the resolved tree itself rather than the pom evidence — let me know
and I'll run it against this head.
**F1 (wait strategy gating).** I traced the actual call order rather than
assuming, and I think the timing concern doesn't apply to this test's path —
happy to be corrected if I'm missing something.
`FakeSourceToConsoleWithEventReportIT` reaches the server through
`SeaTunnelContainer.startUp()` (no-`Network` overload) →
`createSeaTunnelServer()` → `createSeaTunnelServer(NETWORK)`. In that method
(`seatunnel-e2e/seatunnel-e2e-common/.../SeaTunnelContainer.java`):
- line 160: `executeExtraCommands(server);`
- line 162: `server.start();`
`executeExtraCommands` runs *before* `server.start()` on this path, and the
override in `FakeSourceToConsoleWithEventReportIT.executeExtraCommands` (lines
106-117) replaces the container's wait strategy via `container.waitingFor(...)`
on that same `server` instance. Testcontainers uses whatever wait strategy is
set at the time `.start()` is invoked, so the custom `received new worker
register:` strategy is in place before the container actually starts and
blocking `.start()` does poll it.
There is a second overload,
`createSeaTunnelContainerWithFakeSourceAndInMemorySink` (lines 200-244), where
`server.start()` (line 232) does run before `executeExtraCommands` (line 234) —
if that's the method you were looking at, the timing concern is real there, but
that overload isn't in this test's call chain
(`FakeSourceToConsoleWithEventReportIT` never calls it). Let me know if you
were tracing a different path and I'll re-check.
**F2 / F3 (close() races and narrow catch).** All three sub-issues look
resolved on this head, in `JobEventHttpReportHandler.java`:
- Concurrent flush with a still-running scheduled task: `close()` only runs
the final flush `if (schedulerTerminated && !interrupted)` (lines 237-251) — if
the scheduler didn't stop in time it's skipped entirely (logged at line 250),
so the flush and the scheduled `report()` never touch
`committedEventIndex`/`localBuffer` concurrently.
- `evictAll()` not reclaiming in-flight connections: the `finally` block now
calls `httpClient.dispatcher().cancelAll()` before
`httpClient.connectionPool().evictAll()` (lines 253-254), so outstanding calls
are cancelled first and their connections become evictable. This is also
covered by a dedicated regression, `testInterruptedCloseCancelsInFlightRequest`
(`JobEventHttpReportHandlerTest.java` lines 112-142), which asserts the
in-flight call is actually canceled on `close()`.
- Narrow catch: both flush calls in `close()` now catch `Exception` broadly
(lines 241, 246), not just `HazelcastInstanceNotActiveException`/`IOException`.
**F5 (retry test hygiene).** Also resolved on this head, in
`JobEventHttpReportHandlerTest.testRetryAfterHttpFailure` (lines 191-220): both
`takeRequest` calls now pass an explicit `10, TimeUnit.SECONDS` timeout (lines
208-209), the two `Buffer`s are opened in try-with-resources (lines 212-215),
and the test drives the retry by calling `handler.report()` directly rather
than relying on the scheduler, even though the handler is constructed with
`Duration.ofDays(1)`.
**F7 (redirects).** Confirmed already fixed — `createHttpClient()` sets
`.followRedirects(false).followSslRedirects(false)` (lines 273-274), and
`testDoesNotFollowRedirects` (lines 222-259) exercises it end to end with a
real 307 response and asserts the redirect target never receives a request.
**F8 (Content-Type charset).** This one is still genuinely open — I don't
see it addressed in code or in a prior comment. `testReportEvent` (lines
174-175) actually asserts the outgoing header is `application/json;
charset=utf-8`, confirming `RequestBody.create(events, httpMediaType)` does
append the charset since `httpMediaType`
(`MediaType.parse("application/json")`) has none set. I don't have visibility
into what the legacy okhttp 2.7.5 client sent here or what downstream
collectors expect, so I can't tell you myself whether this is a behavior change
that matters. @goutamadwant, could you check what the pre-migration client sent
as `Content-Type` and confirm whether any known collector cares about the exact
header (no charset vs. `; charset=utf-8`)? If it's a real behavior change and
collectors are strict about it, the fix is just constructing `httpMediaType`
with the charset already applied so `RequestBody.create` doesn't need to add
one.
To summarize where this leaves things: F1, F2, F3, F5, F7 check out as
resolved on the current head with the evidence above, F4 is pinned by
construction (pom evidence above, happy to also run `dependency:tree` if you'd
like it), and F8 is the one open item needing either a source dig or a code
change from @goutamadwant.
--
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]
