SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5807076453
Thanks for the detailed write-up. If the production/test diff is indeed unchanged since the last round, the earlier findings still apply. Restating what is needed to move forward: **Blocking (MEDIUM)** - **F1 / F2 – `CoordinatorService.clearCoordinatorService()`**: the description says the coordinator snapshots the instance it owns before any shutdown wait, but as reviewed the method closes whatever the mutable `jobHistoryService` field points to at that moment and leaves the closed instance referenced afterwards. Please either snapshot the field into a local, close that local, and null the field, or point me at the exact code that already does this so I can re-check. Also confirm that every re-activation path unconditionally constructs a fresh `JobHistoryService` (or, better, remove that dependency). - **F3 – `JobHistoryService` constructor**: if the second or third `addEntryListener` throws, the earlier registrations are orphaned and can never be removed by `close()`. Please wrap the registrations so a failure removes what was already registered before rethrowing. **Non-blocking (LOW), still requested** - **F4**: wrap the listener registrations in `testCloseRemovesFinishedJobEntryListeners` in `try/finally` so an assertion failure does not leak listeners into other tests in the shared JVM. - **F5**: in `removeEntryListenerQuietly`, distinguish the expected shutdown case from genuine removal failures (log the latter at a higher level, or include enough context to tell them apart). - **F6**: have `JobHistoryService` implement `AutoCloseable` now that it has a `close()` lifecycle method. - **F7**: annotate `getEntryListenerRegistrationIds()` as `VisibleForTesting`. - **F8**: give `AbstractSeaTunnelServerTest` a type argument in the new test instead of using the raw type. One housekeeping note: the comment above appears to end mid-sentence in the summary section, before the promised F1–F8 breakdown. If there were per-finding responses there, could you re-post them (or reply per point inline)? Otherwise, addressing each item directly in code is the fastest path. Once F1–F3 are fixed I'll do another pass. <!-- streview-comment:1286 --> -- 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]
