DanielLeens commented on PR #11809:
URL: https://github.com/apache/seatunnel/pull/11809#issuecomment-5812736155
Thanks for the follow-up, @SEZ9.
Status check first: the current head
(d7de2afffb7ac7b2f83c486f62c103e13605206f) is a dev-sync merge commit on top of
ab1538a9c1241741940cc789e88fb79bf18df608, not a new production commit. I diffed
it against the current merge-base with dev and the production/test diff is
unchanged: same 4 files (CoordinatorService.java, JobHistoryService.java,
JobHistoryServiceListenerCleanupTest.java,
JobHistoryServiceRegistrationTest.java), same logic, just shifted a couple of
lines from the dev sync. So everything below still applies to this head.
On "comment appears to end mid-sentence": I re-pulled my F1-F5 comment (id
5661717266) straight from the API and it is not truncated, it ends cleanly
after the F5 paragraph. This is the third time we've hit this rendering
artifact (2026-08-25, 2026-09-13, and now), and in the prior two cases it
correlated with markdown links wrapping backtick code and line anchors. To rule
that out for good, here is the same F1-F8 status again, in plain text, no
markdown links this time, re-verified line-by-line against the current head
just now:
F1 - done. CoordinatorService.clearCoordinatorService() snapshots the field
into a local, "JobHistoryService closingJobHistoryService =
jobHistoryService;", right after the CAS guard and before the
executorService.awaitTermination(20s) wait. The close call further down is
closingJobHistoryService.close(), made on that captured local, never on the
field. So a concurrent initCoordinatorService() re-activation that reassigns
the field mid-wait cannot have its fresh instance closed by this teardown; the
snapshot pins cleanup to the outgoing generation.
F2 - done, confirmed safe without nulling. The field is deliberately left
non-null after close (inline comment: "The instance itself is kept because read
paths may still use it until a new active master creates a fresh one."). This
is safe because there is exactly one assignment site for the field in the whole
class, in initCoordinatorService(), guarded by its own comment ("Never reuse
the previous history view") and it unconditionally constructs a brand-new
JobHistoryService on every activation. No re-activation path ever reuses the
field instead of constructing fresh, so a stale-but-closed instance is only
reachable by callers who already held a reference from before the close, which
is the intended "reads still work" behavior, not a re-activation hazard.
F3 - done. JobHistoryService's constructor assigns the three
addEntryListener results (state, metrics, dagInfo) into local variables first,
wraps all three calls in try/catch(RuntimeException), and on failure calls
removeEntryListenerQuietly for whichever of the state/metrics ids were already
acquired before the throw (null-safe, so a failure on the very first call
removes nothing). It then rethrows the original exception. A failed constructor
is never published to the coordinator, so nothing else can observe the
partially-rolled-back state.
F4 - done. testCloseRemovesFinishedJobEntryListeners uses try-with-resources
for both the positive-control instance and each loop-created instance. Since
JobHistoryService implements AutoCloseable, close() runs on any exit path
including an AssertionError, with no explicit try/finally needed.
F5 - done. removeEntryListenerQuietly now has two catch blocks:
HazelcastInstanceNotActiveException logged at fine (expected, node already
stopping), any other Exception logged at warning with the registration id and
store name. It no longer swallows everything at one level.
F6 - done. JobHistoryService implements AutoCloseable, with an overriding
public void close().
F7 - done. getEntryListenerRegistrationIds() is package-private and
annotated @VisibleForTesting.
F8 - done. The test class is declared extends AbstractSeaTunnelServerTest
parameterized with itself, not the raw type.
Re-verified all eight directly against the current head's source just now,
not carried forward from memory or from the commit message. F1-F8 are all
resolved, nothing open on my side. Over to you for the final pass, happy to
answer anything that's still unclear, and I'll avoid markdown links in this
thread going forward to rule that out as a cause.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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]
