SEZ9 commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5826027402
Thanks for the update. Understood that d7de2afffb7ac7b2f83c486f62c103e13605206f is a dev-sync merge on top of ab1538a9c1241741940cc789e88fb79bf18df608 with the production/test diff unchanged, so the earlier points still apply to this head. On the status for the points from comment 5661717266: - F1 (close a snapshot, not the mutable field): capturing the field into a local right after the CAS guard and calling `close()` on that local is the right shape. I'll confirm it against the diff on the final pass. - F2 (closed instance left in the field): the reasoning that the only assignment site is in `initCoordinatorService()` and always constructs a new `JobHistoryService` sounds fine; I'll verify it in the diff as well. - F3 (partial-construction listener leak): the status I can see ends at "assigns the three ad", so I can't tell what was done. Could you briefly restate how the constructor now handles a failure in the second or third `addEntryListener` (i.e. that earlier registrations are removed rather than orphaned)? - F4–F8: I don't see a status for these. Could you post a short line for each (try/finally around listener registration in the test, the swallowed-exception level in `removeEntryListenerQuietly`, `AutoCloseable`, the `VisibleForTesting` annotation on the test accessor, and the raw `AbstractSeaTunnelServerTest` type), even if the answer is "won't change" with a reason? Once F3–F8 are covered I'll do a final pass on this head against the diff. <!-- streview-comment:1310 --> -- 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]
