SEZ9 commented on PR #11814: URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-6621276374
Thanks for pushing this forward — good to see CI green on head `574571ff244847159796a48cef98f0756547d8a3` (apache-side Build `124986677938` is `SUCCESS` and fork run `41352860976` completed). A green build doesn't by itself close out the points from my earlier review, so before merging I'd like to get these resolved or hear why they aren't needed: **Blocking** - **PR11814-F1 (E2E wait strategy)** — In `FakeSourceToConsoleWithEventReportIT`, the wait on `received new worker register` is set inside `executeExtraCommands`, i.e. after the container lifecycle has already been configured/started, so it does not reliably gate server readiness. Please move the wait strategy to where the container is configured, or explain why the current placement is sufficient. **Should fix before merge** - **PR11814-F2 / F3 (`JobEventHttpReportHandler` `close()`)** — When the scheduler fails to terminate, `close()` still flushes the local buffer while the interrupted task may still be running, and `evictAll()` cannot reclaim in-flight connections. The flush is also a blocking Hazelcast/HTTP call on a possibly-interrupted thread, and only `HazelcastInstanceNotActiveException`/`IOException` are caught, so other runtime failures escape `close()`. Please guard the flush on successful termination (or otherwise serialize it with the task) and broaden the exception handling so `close()` cannot throw. - **PR11814-F4 / F6 (Kotlin stdlib graph)** — The compile-scope okhttp3 dependency brings `kotlin-stdlib 1.8.21` alongside `kotlin-stdlib-common 1.9.10` onto the Zeta server runtime classpath without version pinning, and the split is mirrored in `known-dependencies.txt` and the release-docs LICENSE. Please pin the Kotlin stdlib artifacts to a single version and confirm the inventories match what the dist actually resolves. - **PR11814-F5 (`testRetryAfterHttpFailure`)** — Please use a timeout when calling `takeRequest()` so a regression can't hang the build, close the request Buffers, and make the trigger explicit rather than relying on the scheduler firing immediately despite the 1-day interval. **Please confirm (low)** - **PR11814-F7** — The new `OkHttpClient` follows redirects by default, so configured report headers (often API tokens) could be forwarded to a redirect target on another host. Consider disabling redirect following, or confirm this is acceptable. - **PR11814-F8** — okhttp3 `RequestBody.create(String, MediaType)` appends `; charset=utf-8` to the `Content-Type` when the configured media type has no charset. Please confirm the reported header stays byte-identical for existing collectors, or build the body from bytes to keep it unchanged. Once F1 is fixed and F2–F5 are addressed (or explained), I'm happy to do a final pass and merge. <!-- streview-comment:2260 --> -- 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]
