DanielLeens commented on PR #11856:
URL: https://github.com/apache/seatunnel/pull/11856#issuecomment-5503126335
Since this is my own PR, another self-check pass — this one scoped
specifically to the commit I just pushed, `a215916e`. Nobody, including @SEZ9,
has looked at it yet, so I'm not re-litigating the 8 issues from the earlier
rounds; those still stand as resolved.
# What `a215916e` changes
On top of the already-reviewed marker/TTL/atomicity work, this commit:
- Removes `SeaTunnelServer`'s `GracefulShutdownAwareService` implementation
and the `onShutdown()` override added in `f953140e` — the fix @SEZ9 asked for
in Issue 1 of the original review, which stood unchanged for 10 days.
- Adds a hand-rolled `Runtime.getRuntime().addShutdownHook(...)` in
`SeaTunnelServer.init()` (`registerShutdownHook`/`shutdownFromJvmHook`) that
writes the marker and then calls `lifecycleService.shutdown()` directly.
- Sets `hazelcast.shutdownhook.enabled: false` in all 15 shipped Hazelcast
config/doc locations (`config/*.yaml`, `deploy/kubernetes/**`, `docs/en|zh/**`)
to stop Hazelcast's own built-in shutdown hook from running alongside the new
one.
- Changes `seatunnel-cluster.sh`'s foreground path from `java ...` to `exec
java ...`.
- Adds `restoringRunningJobsFromMasterSwitch` /
`canClearGracefulMemberRemovalMarker()` to keep the marker alive through
master-failover job recovery.
That's a much bigger footprint than a log-level fix, so I traced it against
the real Hazelcast source rather than taking the new code comments at face
value.
# Issue 1 (Critical) — the stated reason for the rewrite doesn't match
Hazelcast's actual source, and a one-line fix was already available
**Location:** `SeaTunnelServer.java`
(`registerShutdownHook`/`shutdownFromJvmHook`, new in this commit);
`com.hazelcast.instance.impl.Node` and
`com.hazelcast.spi.properties.ClusterProperty` in `hazelcast/hazelcast` tag
`v5.1` (this project's pinned Hazelcast version — verified directly against
that tag, not from memory).
The new Javadoc says: *"Hazelcast's own hook cannot provide that ordering
because it puts its services into the shutting-down state before
graceful-service callbacks run."* Tracing `Node` at `v5.1` directly:
```text
Node.shutdown(boolean terminate)
[Node.java:502-552]
if (!terminate) callGracefulShutdownAwareServices(maxWaitSeconds)
[Node.java:518-520]
-> GracefulShutdownAwareService.onShutdown()
[Node.java:554-587]
// only afterward: resetJoinState(), removeShutdownHook(),
shutdownServices() teardown
NodeShutdownHookThread.run()
[Node.java:759-778]
switch (policy) {
case TERMINATE: hazelcastInstance.getLifecycleService().terminate(); //
-> Node.shutdown(true), SKIPS graceful callbacks entirely
case GRACEFUL: hazelcastInstance.getLifecycleService().shutdown(); //
-> Node.shutdown(false), same graceful path as any other caller
}
```
Hazelcast's own hook calls the *identical* `Node.shutdown(false)` method as
everything else — there is no separate, earlier-teardown code path, and
`GracefulShutdownAwareService` callbacks run before teardown regardless of what
triggers `.shutdown()`. So "ordering" isn't actually the problem.
The real gap is one property: `ClusterProperty.SHUTDOWNHOOK_POLICY` defaults
to `"TERMINATE"` (`ClusterProperty.java:1533-1534`), and this repo has never
set it to `GRACEFUL` — not on `dev`, not anywhere in this PR before this commit
(`git grep shutdownhook` on both only turns up `shutdownhook.enabled`, added
today). So Hazelcast's own default hook, if it's what actually answers a
container's SIGTERM, calls `.terminate()` — skipping
`callGracefulShutdownAwareServices()` altogether. That is a real,
previously-unnoticed gap in `f953140e`: all 8 prior self-review rounds and
@SEZ9's review verified that `onShutdown()` runs before teardown *once
invoked*, but never checked whether Hazelcast's default configuration would
invoke the graceful path at all on a real signal.
Given that, the minimal fix is:
```yaml
hazelcast.shutdownhook.policy: GRACEFUL
```
added next to the existing `properties:` blocks — zero Java changes — which
makes Hazelcast's own already-mature, already-self-deregistering hook call
exactly the `f953140e`/`94eaf4155b` mechanism that's already been reviewed and
had its race condition fixed. That keeps this PR's Java diff where it was after
`94eaf4155b`, with no new hook-vs-hook interaction to reason about.
# Issue 2 (High) — the chosen fix creates a new dual-hook race for any
deployment that doesn't carry the new config forward
The docs added in this commit say: *"Custom Hazelcast configurations must
retain that setting to preserve this behavior."* Nothing in the code enforces
or checks that. If a deployment supplies its own Hazelcast properties — an
ordinary scenario for a production system customizing vendor YAML over years —
without `hazelcast.shutdownhook.enabled: false`, both hooks get registered:
- Hazelcast's own `NodeShutdownHookThread` (default policy `TERMINATE`)
- SeaTunnel's new `seatunnel-graceful-member-removal-shutdown` thread
Per the JVM's own `Runtime.addShutdownHook` contract, registered hooks are
started in unspecified order and run concurrently. `Node.shutdown()` has a
re-entrancy guard (`setShuttingDown()`), so whichever thread's call wins that
race decides graceful vs. forceful — which is scheduler-dependent, not
deterministic. If Hazelcast's own hook wins, the instance tears down via
`.terminate()` while SeaTunnel's hook may not have written the marker yet,
reproducing the exact ERROR-instead-of-WARN symptom this PR exists to fix —
silently, with no test able to catch a JVM-shutdown-hook race. That's a
production-only failure mode, gated purely on whether an operator's Hazelcast
config happens to carry a doc-only convention forward.
# Non-blocking, but worth recording
- `exec java ...` in `seatunnel-cluster.sh` (foreground path) is a good fix
on its own merits and worth keeping regardless of how Issues 1-2 are resolved:
container runtimes typically deliver SIGTERM to PID 1 only, and without `exec`
the JVM here runs as a child of the launcher shell that may never see it
directly.
- `restoringRunningJobsFromMasterSwitch` /
`canClearGracefulMemberRemovalMarker` (retaining the marker through
master-failover job recovery) reads correctly to me on a source-level pass, but
it's new, untested-by-anyone-else surface added the same day as Issues 1-2, on
an already large PR.
- CI on this exact head (`a215916e`) is still queued as of this review — no
build/test signal either way yet.
# Recommendation
Not my call to make alone — that's the actual problem with this being round
9 of what's mostly been me reviewing my own PR. @SEZ9, given this commit swaps
out the mechanism you already signed off on for a new one with the race
described in Issue 2, could you take a look specifically at `a215916e` when you
have a chance? I'd lean toward reverting to the `94eaf4155b` shutdown mechanism
plus the one-line `hazelcast.shutdownhook.policy: GRACEFUL` config addition
(Issue 1) rather than keeping the new custom hook, but that's a real design
call and I don't think I should be the one deciding it on my own PR.
--
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]
