DanielLeens commented on PR #11814:
URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-6266725003
Thanks @SEZ9 for going back to the actual head. No new commit has landed
since my last review at `131de7f704`, so I re-read the live source at that
exact SHA and am pointing at the lines for each item (all in the PR diff,
`JobEventHttpReportHandler.java` unless noted):
1. **F1 - E2E wait.** `FakeSourceToConsoleWithEventReportIT.java:113-125`
sets `container.waitingFor(...)` with the `.*received new worker register:.*`
regex from inside `executeExtraCommands` (starts at line 95). The comment at
lines 111-112 records the ordering: this test goes `startUp() ->
createSeaTunnelServer()`, which calls the hook *before* `start()`, so the
strategy does gate container startup. The wrapper only adds a thread dump
(`logStartupThreads`, line 129) on a failed wait and rethrows.
2. **F2/F3 - `close()`.** Lines 214-270. `closing` is set first (line 216)
and `handle()` returns early on it (line 125). The final flush at line 238 runs
only if `schedulerTerminated && !interrupted`; otherwise it is skipped with a
warning (line 261), so the buffer flush cannot overlap a still-running task.
`finally` (lines 263-269) always runs `dispatcher().cancelAll()` and
`connectionPool().evictAll()` and restores the interrupt flag. Wrapped failures
are unwrapped via `CompletionException` (lines 243-246); only
`HazelcastInstanceNotActiveException` is logged at INFO, everything else goes
to ERROR (lines 247-253), and the local buffer flush runs regardless (line
256). Tests: `testInterruptedCloseCancelsInFlightRequest` (test:127),
`testCloseWhenHazelcastNotActive` (test:347),
`testCloseLogsUnexpectedRingbufferFailure` (test:398).
3. **F4/F6 - Kotlin.** `seatunnel-engine-server/pom.xml` imports
`kotlin-bom` (line 34) and declares `kotlin-stdlib-jdk8` without a version
(line 114), so the whole stdlib family resolves to one version.
`known-dependencies.txt` lines 77-80 list `kotlin-stdlib`, `-common`, `-jdk7`,
`-jdk8` all at `1.9.10`, and `release-docs/LICENSE` lines 258-261 list the same
four at `1.9.10` (okhttp/okio at 262-264).
4. **F5 - retry test.** `testRetryAfterHttpFailure` (test:207) uses
`Duration.ofDays(1)` (line 218) so no periodic tick can drive the retry, bounds
both `takeRequest` calls at 10 seconds (lines 223-224), and both recorded
request `Buffer`s are closed via try-with-resources (lines 227-230).
5. **F7 - redirects.** Lines 284-285: `.followRedirects(false)` and
`.followSslRedirects(false)`; `testDoesNotFollowRedirects` (test:238) asserts
the redirect target never receives a request.
6. **F8 - Content-Type.** `postEvents` (lines 192-196) uses
`RequestBody.create(events, httpMediaType)` with
`MediaType.parse("application/json")`. `testReportEvent` asserts the header is
`application/json; charset=utf-8` (test:190); okhttp3 adds the charset for a
String body the same way the 2.x call did, so existing collectors see the same
header. The user-visible behaviour changes that remain (redirects not followed,
modern TLS defaults only) are documented in `incompatible-changes.md` (en and
zh) and `event-listener.md`.
On the CI side, the required `Build` is `SUCCESS` for this head. I did not
run any of this locally; the above is a source-level read at `131de7f704`.
@goutamadwant nothing further is needed from you on these items from my side.
Please let me know if any line reference does not match what you see on your
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]
