DanielLeens commented on PR #12984:
URL: https://github.com/apache/seatunnel/pull/12984#issuecomment-6396547002
Maintainer-style technical assessment of
`a5645167aa82152d823bdcefe3f0596a513ce5ac`. Daniel is this PR's author, so this
is a self-authored PR assessment, not an independent approval. No earlier
community review or inline comment was present. Report time:
2026-10-25T06:32:37.754079-04:00.
# What Problem Does This PR Solve?
- User pain point: every master activation creates a shared connector-JAR
cleanup timer, but demotion did not release it. Repeated master changes retain
obsolete timer threads and their referenced server/map objects.
- Fix approach: give the storage strategy an additive close contract, cancel
its timer on coordinator teardown, and cancel a newly started timer if
scheduling fails.
- One-sentence summary: make the shared JAR cleanup timer follow the
active-master service lifecycle.
Example: after master A steps down and later becomes master again, it should
own one current cleanup timer, not its current timer plus the previous
activation's timer.
# 1. Code Change Review
## 1.1 Core Logic Analysis
Full diff: 5 files, +115/-11. Key locations:
`CoordinatorService.java:1349-1355`,
`ConnectorJarStorageStrategy.java:115-118`,
`ConnectorPackageService.java:126-131`,
`SharedConnectorJarStorageStrategy.java:62-75,157-161`, and
`ConnectorPackageServiceTest.java:143-241`.
Before:
```java
this.cleanupTimer = new Timer(true);
this.cleanupTimer.schedule(task, cleanupInterval, cleanupInterval);
// Coordinator teardown never closed the package service.
```
After:
```java
Timer timer = new Timer(CLEANUP_TIMER_THREAD_NAME, true);
try { timer.schedule(task, cleanupInterval, cleanupInterval); }
catch (RuntimeException e) { timer.cancel(); throw e; }
// After the existing executor termination wait:
ConnectorPackageService packageService = connectorPackageService;
connectorPackageService = null;
if (packageService != null) { packageService.close(); }
```
The snippets abbreviate unchanged task-construction arguments; the complete
diff was read.
Key findings:
- The normal enabled-SHARED master lifecycle reaches this code; disabled
storage and ISOLATED storage do not create this timer.
- Both successful teardown and constructor scheduling failure now release
the owned timer.
- Activation and teardown share the coordinator monitor
(`CoordinatorService.java:1263,1303`), preventing the next activation from
interleaving with this close.
- The package service remains reachable during the existing 20-second
coordinator-executor termination wait. This preserves the normal
`JobMaster.run` finalization lookup.
- `Timer.cancel()` prevents future tasks; it does not interrupt an already
executing cleanup task. This patch is resource cleanup, not distributed fencing
of a running deletion.
Complete affected runtime path:
```text
checkNewActiveMaster (1263-1275)
-> initCoordinatorService (622-666), when jar storage enabled
-> ConnectorPackageService constructor (57-67)
-> StorageStrategyFactory.of -> SharedConnectorJarStorageStrategy (56-75)
-> Timer -> SharedConnectorJarCleanupTask.run -> deleteConnectorJar
master demotion / activation failure / server shutdown
-> clearCoordinatorService (1303)
-> interrupt JobMasters, shutdown coordinator executor, await <=20 seconds
JobMaster.run finally (766-774) -> getConnectorPackageService -> release
jar refs
-> detach connectorPackageService -> close -> strategy.close -> Timer.cancel
-> resource manager and event processor cleanup
schedule failure in constructor -> cancel local timer -> rethrow original
exception
```
The fix belongs to service teardown, not individual job completion: shared
storage outlives any one job. The existing bounded-wait behavior means a
JobMaster that ignores interruption past the timeout is outside the
orderly-finalization guarantee; this review does not claim otherwise.
## 1.2 Compatibility Impact
Fully compatible for the traced runtime contracts. Existing strategy
implementations inherit a no-op `close`; ISOLATED behavior is unchanged. No
option, default, JAR layout, ref-count protocol, checkpoint/savepoint format or
job-state model changes. Strategies are local service objects in the inspected
call graph, not checkpoint payloads; this is not a claim that arbitrary
external Java serialization of those implementation objects is supported.
## 1.3 Performance / Side-Effect Analysis
The change removes retained timer threads and associated heap references
after ownership ends. No row hot-path CPU, GC, network, lock-contention, retry
or idempotency change. Closing twice is safe for `Timer.cancel`. The
pre-existing cleanup callback and distributed ref-count logic remain unchanged;
close does not delete shared metadata or JARs. An already running timer task
may finish after cancellation.
## 1.4 Error Handling and Logging
No blocking source issue found. Scheduling failures retain their original
exception after timer cancellation. Built-in close implementations do not
throw. Existing coordinator timeout/interruption logging and subsequent service
cleanup are retained. No new credentials or high-volume logs are introduced.
# 2. Code Quality Assessment
## 2.1 Coding Standards
The change stays within the timer owner, its service wrapper and coordinator
lifecycle. New lifecycle hooks and the named timer field explain ownership and
purpose. No dependency or registration change is needed.
## 2.2 Test Coverage and Test Stability
**Stability rating: Stable under the repository's default non-parallel JUnit
execution.** `ConnectorPackageServiceTest.java:143-212` retains the existing
real two-node failover assertions and adds condition-polled counts before/after
master shutdown and final shutdown. Lines 217-241 exercise invalid scheduling
and await return to the timer baseline. Both tests have failure power: removing
teardown cancellation leaves the old named timer; removing the constructor
catch leaves a timer after rejected scheduling. Assertions were added, not
weakened.
The fixture explicitly enables SHARED storage, uses a long cleanup interval,
and shuts both nodes down in `finally`. No new port, sleep, random expected
value or external service is added. The process-wide thread count assumes no
independently parallel test creates the same named timer; enabling such
parallelism would require isolation or a narrower ownership probe. These are
embedded-cluster integration checks in the existing test class, not a claim of
a separate E2E run.
Local verification results:
- PR fetch and branch `seatunnel-review-12984`: succeeded; local HEAD
matches the stated SHA.
- Complete merge-base diff/stat: all 5 files read, +115/-11.
- `rg` and source reads: checked activation, all teardown callers, strategy
factory, cleanup task and JobMaster finalization.
- `git diff --check upstream/dev...HEAD`: passed.
- Compilation, UT, IT, E2E and actual service execution: not run locally, as
requested; GitHub CI remains the runtime gate.
## 2.3 Documentation Updates
No user-facing option or behavior contract changes, so neither EN nor ZH
needs a migration entry. Existing config defaults remain intact.
# 3. Architectural Soundness
## 3.1 Elegance
Precise lifecycle fix. The root cause is missing resource ownership release
between `CoordinatorService` activations; the timer itself belongs to
`SharedConnectorJarStorageStrategy`. The current placement is best: the
coordinator closes its service, which closes its strategy. A global timer
registry or per-job cancellation would add shared state or prematurely stop
cleanup. The smallest safe and global best changes coincide for this leak.
## 3.2 Maintainability
The default close method keeps strategies source/binary compatible and
centralizes future strategy resource cleanup.
## 3.3 Extensibility
Other strategies can implement close without changing coordinator
storage-mode logic.
## 3.4 Historical-Version Compatibility
Existing jobs, configuration, state recovery and JAR identifiers remain
unchanged; no migration is required. Distributed fencing and pre-existing
cleanup/ref-count races are not redesigned here.
# 4. Issue Summary
Completeness cross-check: all changed files, resource lifecycle, call chain,
constructor failure, compatibility, test failure power/stability, docs and live
gates were covered. Runtime execution remains unverified locally by policy.
| Number | Issue | Location | Severity |
|---|---|---|---|
| - | No blocking source issue found | lifecycle and timer locations above |
- |
# 5. Merge Recommendation
### Conclusion: Ready to merge after checks pass
1. **Blockers (must resolve):** no completed current-head CI evidence is
available yet; required checks and an independent write-capable maintainer
review remain necessary. Daniel cannot approve his own PR. GitHub currently
reports `BLOCKED` / `REVIEW_REQUIRED`, no merge conflict; compare is diverged,
ahead 1 / behind 1. Drift is recorded only, with no unrelated failing check to
justify a sync-first request.
2. **Recommended fixes (non-blocking):** none required for this focused leak
correction.
Overall assessment: the resource owner now has an explicit teardown path and
constructor-failure cleanup. No broader implementation is needed for this
scoped leak. There is no earlier maintainer opinion to reconcile. This
self-authored technical assessment does not supply independent approval, CI
success or production runtime validation.
--
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]
