SEZ9 commented on PR #11814: URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-5825773436
Thanks for the detailed follow-up on `ec6a7f9affc`. **F8** — The explanation makes sense: if the okhttp3 `RequestBody.create(String, MediaType)` overload adds `; charset=utf-8` when the `MediaType` has none, and the pre-PR okhttp 2.x call did the same, then the header existing collectors see is preserved rather than changed by this migration. I haven't verified that against the diff myself, so I'll treat this as pending until I can confirm it in the code. **F5 / F7** — Likewise, the described changes (explicit `handler.report()` calls instead of relying on the `Duration.ofDays(1)` interval, `takeRequest` bounded at `10, TimeUnit.SECONDS`, request `Buffer`s closed via try-with-resources; `.followRedirects(false).followSslRedirects(false)` covered by `testDoesNotFollowRedirects` with a 307) would address the findings. I'd like to confirm them against the diff before closing them out. I don't see updates on the remaining points yet — could you point me to the change or share your plan? 1. **F1 (HIGH)** — The E2E wait on `received new worker register` is applied in `executeExtraCommands`, after the container lifecycle is already configured/started, so it doesn't reliably gate readiness. Please move it into the container wait-strategy setup. 2. **F2 (MEDIUM)** — When the scheduler doesn't terminate in time, `close()` still flushes the local buffer while the interrupted task may still be running, and `evictAll()` can't reclaim in-flight connections. Please guard the final flush on successful termination or otherwise serialize it with the running task. 3. **F3 (MEDIUM)** — `close()` performs blocking Hazelcast/HTTP work on a possibly-interrupted thread and only catches `HazelcastInstanceNotActiveException`/`IOException`, so other runtime failures can escape. Please broaden the handling so `close()` never throws. 4. **F4 (MEDIUM) / F6 (LOW)** — Compile-scope okhttp3 brings a split Kotlin stdlib graph (kotlin-stdlib 1.8.21 vs kotlin-stdlib-common 1.9.10) onto the Zeta server classpath, and the same split appears in the dependency and license inventories. Please pin the Kotlin stdlib to a single version, or confirm the resolved dist artifacts and align the inventories to match. Happy to take another look once these are addressed. <!-- streview-comment:1298 --> -- 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]
