DanielLeens commented on PR #11809: URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5846259034
Thanks for confirming F1 and F2 look right in shape — I'll wait for your check against the diff itself on those two rather than repeating them here. On the repeated cut-off: I just re-pulled my last comment (id 5812736155) directly from the API rather than the rendered page. It is 4292 bytes and ends cleanly on the attribution line after the F8 sentence — nothing is missing from the stored body. This is now the fourth time this has happened on this thread (2026-08-25, 2026-09-13, 2026-09-24, and now), always on a numbered F-list that follows backtick-wrapped generic types or markdown links. So let me try the plainest possible format this time: no backticks around generics, no links, one short paragraph per item. F3: The constructor assigns the three addEntryListener results, for the state map, the metrics map, and the DAG info map, into local variables first, wraps all three calls in one try block, and catches RuntimeException around them. On failure it calls removeEntryListenerQuietly for whichever of the state and metrics ids were already acquired before the throw. That helper is null-safe, so a failure on the very first call removes nothing extra. The original exception is then rethrown unchanged. A failed constructor is never published to the coordinator, so nothing downstream can ever observe a partially registered instance. F4: testCloseRemovesFinishedJobEntryListeners now creates both the positive-control service and each loop-created service inside try-with-resources instead of a plain local variable. Since JobHistoryService now implements AutoCloseable, close runs on every exit path including an assertion failure inside the try block, so a separate try/finally is not needed once try-with-resources is used. F5: removeEntryListenerQuietly now has two catch blocks instead of one. HazelcastInstanceNotActiveException is caught first and logged at fine level, since that's the expected case while the node is already stopping. Any other exception falls into a second catch and is logged at warning level together with the registration id and the IMap name, so a genuine removal failure is distinguishable from an expected shutdown exception in the logs. F6: JobHistoryService now declares implements AutoCloseable and has an overriding public close method. F7: getEntryListenerRegistrationIds is package-private and carries the VisibleForTesting annotation. F8: The test class now extends AbstractSeaTunnelServerTest parameterized with itself, JobHistoryServiceListenerCleanupTest, instead of using the raw type. I re-read all six of these directly against the current head's source just now rather than copying them from an earlier comment, so they reflect the code exactly as it stands on d7de2afffb7ac7b2f83c486f62c103e13605206f. If this still gets clipped on your side, let me know and I'll split F3 through F8 into six separate single-item comments instead — that would rule out any per-comment-length trigger for good. -- 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]
