SEZ9 commented on PR #11814: URL: https://github.com/apache/seatunnel/pull/11814#issuecomment-5650998830
Thanks for the update on the delta and the CI picture. On the `af713e463f` vs `9e9dd08f9a` delta: agreed that it is limited to the `seatunnel-starter/pom.xml` test-scope cleanup and the `HttpReportPackagingIT.java` relocation into `connector-console-seatunnel-e2e`. I also take the point that the `minio/minio` pull-access-denied failures on run `34619448739` are in modules this PR does not touch, and that `engine-v2-it (8, ubuntu-latest)` / `engine-v2-it (11, ubuntu-latest)` are reported green there. That said, a green `engine-v2-it` does not by itself close out the earlier review items, since none of them were about the packaging test or CI completion. Before I can approve, could you confirm the status of each of these on the current head: 1. **PR11814-F1 (HIGH, Test)** – `FakeSourceToConsoleWithEventReportIT`: the wait strategy on `received new worker register` is still set in `executeExtraCommands`, after the container lifecycle is configured, so it does not reliably gate server readiness. A passing run does not demonstrate this is fixed; please move the wait into the container configuration (or explain why the current placement is sufficient). 2. **PR11814-F2 / F3 (MEDIUM, Robustness)** – `close()` in `JobEventHttpReportHandler.java`: (a) when the scheduler does not terminate, the local buffer is still flushed concurrently with the interrupted-but-running task and `evictAll()` cannot reclaim in-flight connections; (b) the blocking Hazelcast/HTTP flush runs on a possibly-interrupted thread and only `HazelcastInstanceNotActiveException`/`IOException` are caught. Has either of these changed? 3. **PR11814-F4 / F6 (MEDIUM/LOW, Compatibility & Docs)** – compile-scope okhttp3 pulling a mixed Kotlin stdlib graph (`kotlin-stdlib` 1.8.21 vs `kotlin-stdlib-common` 1.9.10) onto the Zeta server runtime classpath without pinning, and the same split in `tools/dependencies/known-dependencies.txt` and `seatunnel-dist/release-docs/LICENSE`. Please confirm the inventories match the actual resolved dist and consider aligning the versions. 4. **PR11814-F5 (MEDIUM, Test)** – `testRetryAfterHttpFailure`: no-timeout `takeRequest()` (possible build hang), leaked request `Buffer`s, and the implicit assumption that the scheduler fires immediately despite the 1-day interval. 5. **PR11814-F7 (LOW, Security)** – the new `OkHttpClient` follows redirects by default, so configured report headers can be forwarded to a redirect target on another host. Please consider disabling redirect following or documenting the behavior. 6. **PR11814-F8 (LOW, Functional)** – `RequestBody.create(String, MediaType)` appends `; charset=utf-8` when the configured media type has no charset; please confirm the emitted `Content-Type` remains byte-identical for existing collectors. If any of these were already addressed in an earlier commit, a short pointer to where is enough. Otherwise a follow-up push covering F1, F2/F3 and F5 at minimum would let me move this forward. <!-- streview-comment:1017 --> -- 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]
