SEZ9 commented on PR #11814: URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-6538000365
Thanks for the work so far. A quick status update for head `ee727193b9e4c46fc34dc58c14c7024b658a4c93`: **CI** [Build](https://github.com/apache/seatunnel/runs/121067566561) is `FAILURE`, with [Run / unit-test (17, ubuntu-latest)](https://github.com/goutamadwant/seatunnel/actions/runs/40159375093/job/121074355248) failing on `seatunnel-engine-server` with surefire test failures. Since `JobEventHttpReportHandlerTest` lives in that module, could you check the surefire report for that job and share which test(s) failed? If `testRetryAfterHttpFailure` is among them, note that PR11814-F5 already flagged its no-timeout `takeRequest()` and its assumption that the scheduler fires immediately despite the 1-day interval; adding a timeout, closing the request `Buffer`s, and triggering the flush deterministically should make it stable. **Merge conflict** GitHub reports conflicts with `dev`. Please sync the latest base and resolve them; the new head will need a fresh CI run and re-review. **Points from the [previous review](https://github.com/apache/seatunnel/pull/11814#pullrequestreview-5643605276)** Please address or reply to each: - PR11814-F1 (HIGH): the `received new worker register` wait strategy in `FakeSourceToConsoleWithEventReportIT` is set in `executeExtraCommands`, after the container lifecycle is configured, so it does not reliably gate server readiness. Please move it into the container setup. - PR11814-F2 / PR11814-F3 (MEDIUM): when the scheduler fails to terminate, `close()` still flushes the local buffer concurrently with the interrupted-but-running task, `evictAll()` cannot reclaim in-flight connections, and only `HazelcastInstanceNotActiveException`/`IOException` are caught, so other runtime failures escape `close()`. Please serialize the final flush against the scheduled task and broaden the exception handling. - PR11814-F4 / PR11814-F6 (MEDIUM/LOW): compile-scope okhttp3 brings a mixed 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 `known-dependencies.txt` and `seatunnel-dist/release-docs/LICENSE`. Please pin to a single version and confirm the inventories match what the dist actually resolves. - PR11814-F7 (LOW): the new `OkHttpClient` follows redirects by default, so configured report headers (often API tokens) could be forwarded to another host. Please disable redirect following or explain why it is acceptable here. - PR11814-F8 (LOW): `RequestBody.create(String, MediaType)` appends `; charset=utf-8` when the configured media type has no charset. Please confirm the emitted `Content-Type` stays byte-identical for existing collectors, or build the body from bytes with the exact `MediaType`. Once the conflicts are resolved and the unit-test job is green, I'll re-review the new head. Thanks! <!-- streview-comment:2158 --> -- 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]
