SEZ9 commented on PR #11814: URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-6567804078
Quick status update on this PR. GitHub currently reports content conflicts with `dev` on the current head `1e406dee272e17471241bfa43eda786ab48eb738` (`CONFLICTING` / `DIRTY`), and the [Build](https://github.com/apache/seatunnel/runs/123937253173) run on that head is `TIMED_OUT`. Could you rebase onto the latest `dev`, resolve the conflicts while keeping both this PR's intended change and the intervening upstream changes, and push so the required checks can run on the new head? Please don't change the reporter semantics just to chase the integration-lane timeout — that is a separate execution gate. The [earlier source review](https://github.com/apache/seatunnel/pull/11814#pullrequestreview-5721025746) still stands as the source assessment, so no new findings here. The items from that review that are still open and that I'd like addressed (or explicitly answered) on the resulting head: - **Test (HIGH)** – `FakeSourceToConsoleWithEventReportIT`: the wait strategy on `received new worker register` is set in `executeExtraCommands`, after the container lifecycle is already configured/started, so it doesn't reliably gate server readiness. - **Robustness** – in the HTTP report handler's close path: when the scheduler fails to terminate, the local buffer is still flushed concurrently with the interrupted-but-running task, and `evictAll()` cannot reclaim in-flight connections; the blocking Hazelcast/HTTP flush also runs on a possibly-interrupted thread and only `HazelcastInstanceNotActiveException`/`IOException` are caught, so other runtime failures escape. - **Compatibility / Docs** – compile-scope okhttp3 pulls a mixed Kotlin stdlib graph (kotlin-stdlib 1.8.21 vs kotlin-stdlib-common 1.9.10) onto the Zeta server classpath without version pinning, and the license/dependency inventories show the same split. Please confirm they match the actual resolved dist and consider aligning the versions. - **Test** – `testRetryAfterHttpFailure` uses no-timeout `takeRequest()` (the build can hang), leaks the request Buffers, and assumes the scheduler fires immediately despite a 1-day interval. Please bound the wait, close the buffers, and trigger the flush deterministically. - **Security (LOW)** – the new `OkHttpClient` follows redirects by default, so configured report headers (often API tokens) can be forwarded to a redirect target on another host. Consider disabling redirect following. - **Functional (LOW)** – `RequestBody.create(String, MediaType)` appends `; charset=utf-8` to the Content-Type when the configured media type has no charset. Please confirm the header sent to existing collectors stays byte-identical, or document the change. Once the rebase is pushed and checks are green, I'll take another look at the resulting head. Thanks for the continued work on this! <!-- streview-comment:2209 --> -- 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]
