nzw921rx commented on PR #12943:
URL: https://github.com/apache/seatunnel/pull/12943#issuecomment-6333111413
## Architecture review — not ready to merge
Scope: static review of `a5230b0` against `upstream/dev@ee83c538` (64 files,
+5736/−187). I did not run the full build or test suite.
The documentation and test coverage are unusually thorough, and the failure
modes are clearly described. The concern is implementation boundaries, not
feature intent. The current shape introduces a second Application runtime and a
second resource-manager model, and threads an HA boolean through generic
execution code. That is the main architecture debt.
---
### P0 — blocking
#### 1. The PR contains #12942's connector changes
**Where**
-
`seatunnel-connectors-v2/connector-file/connector-file-base/src/main/java/org/apache/seatunnel/connectors/seatunnel/file/source/split/MultipleTableFileSourceSplitEnumerator.java`
-
`seatunnel-connectors-v2/.../MultipleTableFileSourceSplitEnumeratorTest.java`
- `seatunnel-e2e/.../connector-file-local-e2e/.../LocalFileIT.java`
- `seatunnel-e2e/.../local_file_bounded_restore.conf`
- `docs/{en,zh}/connectors/source/LocalFile.md`
The PR body says the branch includes #12942 and will be rebased after it
lands; #12942 is still open. These files are 381 lines unrelated to Kubernetes
HA.
**Why it matters**
The HA change cannot be reviewed, merged, reverted, or bisected
independently of the connector fix. This violates the repository's
one-PR-one-problem boundary.
**Recommendation**
Land #12942 first, rebase this branch, and remove those files from this PR.
---
### P1 — high priority
#### 2. HA is a second Application runtime, not a strategy
**Where**
- Single-master: `SeatunnelKubernetesApplicationCli.java:51-72, 104-126,
185-206` uses `KubernetesResourceManagerDriver` + `ApplicationJobRunner`.
- HA: `SeatunnelKubernetesHaApplication.java:66-117, 122-311` uses
`FixedPoolApplicationResourceManager` + `SeaTunnelClient` + a temporary job
file.
- Entry selection: `KubernetesApplicationClusterDescriptor.java:146-151`.
**What is duplicated**
- Job submission: `ApplicationJobRunner:66-126` vs
`SeatunnelKubernetesHaApplication:206-245`.
- Terminal mapping: `ApplicationJobRunner:258-265` vs
`SeatunnelKubernetesHaApplication:422-430`.
- Status persistence: `ApplicationResourceManager.finish` vs owner Job
annotations.
- Resource management: `ApplicationResourceManager` (`:48-58, 177-260`)
already owns worker registration, failure propagation, and `finish`;
`FixedPoolApplicationResourceManager:32` instead extends
`StandaloneResourceManager` and only overrides the worker-count gate. HA
cleanup is reimplemented in `SeatunnelKubernetesHaApplication:432-444`.
- Mode selection: `ResourceManagerFactory.java:28, 61-75` adds a mutable
`externallyManagedWorkers` field outside `DeployType`.
**Why it matters**
The same application configuration now has two different submission,
restore, status, and cleanup contracts. A fix in one path does not
automatically apply to the other. `FixedPoolApplicationResourceManager` also
loses the application identity, failure future, and finish semantics provided
by `ApplicationResourceManager`.
**Recommendation**
Extract a provider-neutral `ApplicationRuntime`/`ApplicationLifecycle`
(submit, observe, terminal, cleanup) with `SingleMasterRuntime` and
`HaObserverRuntime`. On the resource side, keep the
`ApplicationResourceManager` contract and add a controller-owned driver instead
of extending `StandaloneResourceManager`. Keep one launcher and select the
runtime by strategy.
#### 3. `applicationHaEnabled` enters the generic execution state machine
**Where**
- `EngineConfig.java:94-95` adds the boolean.
- `SeatunnelApplicationConfig.java:337-350` sets it and rewrites Hazelcast
map backups / schedule strategy.
- `SubPlan.java:413-421, 514-516, 749-758, 778-813`.
- `JobMaster.java:340-361, 531-629`.
- `CheckpointManager.java:163-195`.
**What changes**
`SubPlan` now uses `isApplicationHaEnabled()` to change:
- whether `forceStopPipeline` takes `synchronized (this)`;
- whether restore sleeps before retrying;
- whether FAILED/CANCELED uses the normal `preApplyResources` path or an
asynchronous HA restore loop;
- whether a shared-executor loop sleeps and polls in
`restoreHaPipelineWhenResourcesAvailable`.
There is also a concrete API hazard: `JobMaster.preApplyResources(SubPlan)`
(`:527-529`) always passes `recoveryStillCurrent = null`. With HA enabled it
takes the non-HA branch at `:619-621` and does **not** call
`setOwnedSlotProfiles`. Correct HA behavior depends on every caller remembering
the two-argument overload at `:532`.
`configureHa` also sets the flag on lite workers and on the owner client
(`SeatunnelKubernetesApplicationWorker.java:50-56`,
`SeatunnelKubernetesHaApplication.java:97-103`), even though it is a
coordinator recovery behavior.
**Recommendation**
Replace the boolean with an explicit
`CoordinatorRecoveryPolicy`/`ApplicationExecutionPolicy` injected into
`JobMaster` and `SubPlan`. Keep `SubPlan` state transitions mode-agnostic. At
minimum, make `preApplyResources(SubPlan)` safe under HA and restrict the flag
to coordinators.
#### 4. `CheckpointStorage` SPI is widened with a default-throwing method
**Where**
- `CheckpointStorage.java:86-98`: new `getCheckpointsForRecovery`, default
implementation throws `CheckpointStorageException`.
- `CheckpointManager.java:219-242`: HA restart calls it.
- `HdfsStorage.java:236-289` and `LocalFileStorage.java:115-171`: each
duplicates the retry/snapshot scan.
**Trigger and result**
A custom checkpoint storage plugin, or a test double, can work for normal
jobs but throws `"Checkpoint storage does not support fail-closed recovery"` on
the first HA master failover. There is no capability check at submission time,
so the failure appears during recovery rather than deployment.
`architecture.md:197-199` already documents the custom-plugin requirement,
while the PR checklist marks `incompatible-changes.md` as N/A.
The new method also overlaps with the existing
`getCheckpointsByJobIdAndPipelineId` (`:86`), so callers must know which
semantics to use.
**Recommendation**
Introduce a `RecoverableCheckpointStorage` capability interface, or validate
support when HA is submitted. Move the retry scan into
`AbstractCheckpointStorage`. Also, `CheckpointManager:230,238` deserializes the
same state twice; cache the decoded checkpoint.
#### 5. HA deployment depends on the submitter's local `seatunnel.yaml`
**Where**
- `KubernetesApplicationConfig.java:140-148`
- `KubernetesOptions.java:42-47`
When `kubernetes.ha.backup-count` is unset, the
`KubernetesApplicationConfig` constructor calls
`ConfigProvider.locateAndGetSeaTunnelConfig()` and reads the submitter
machine's `seatunnel.engine.backup-count`.
**Why it matters**
The same `application.config` submitted from two machines with different
local `seatunnel.yaml` files produces different `haBackupCount` values and
different Hazelcast map replication. The value is silently embedded in the Job
spec; it is not part of the application configuration and is not reviewable.
The application spec is no longer a self-contained, reproducible source of
truth.
**Recommendation**
Require an explicit `kubernetes.ha.backup-count`, or resolve it from the
runtime `seatunnel.yaml` actually mounted into the masters and write the
resolved value into the spec. Do not read local engine configuration inside a
value-object constructor.
#### 6. Single-master `cancel` behavior regressed when the Job is absent
**Where**
- `KubernetesApplicationClient.java:119-137`
- `KubernetesApplicationClusterDescriptor.java:484-488`
- The new behavior is codified by `KubernetesApplicationTest.java:593-598`.
The new path calls `getJob` first; on 404 it sets `canceled = true` and
returns. The base path (`KubernetesApplicationClusterDescriptor:450-453` plus
`DefaultKubernetesClient.deleteApplication:250-279`) continued to delete
worker/master Pods by label.
**Trigger**
The Job was deleted by TTL or foreground deletion, but labeled Pods remain
(or are still being deleted). `cancel` now reports `CANCELED` without cleaning
them up.
**Why it matters**
This contradicts "existing single-master defaults and standalone behavior
are unchanged" and can leak compute resources. It affects the shared
single-master path, not only HA.
**Recommendation**
For non-HA, preserve the label-based fallback when the Job is 404. Only skip
cleanup when HA is enabled and the owner UID is genuinely unknown, or provide
explicit orphan cleanup.
---
### P2 — design and boundary cleanup
#### 7. Role Pods are assembled by clone-and-mutate; master-only
configuration leaks
**Where**
- `HaApplicationFactory.java:92-150` builds master and worker from
`ApplicationJobFactory.createMasterPod`, then overwrites them.
- `HaApplicationFactory.java:152-179` builds the owner from the same master
factory.
- `ApplicationJobFactory.java:117-131` adds the checkpoint PVC to every
master Pod.
- `HaApplicationFactory.java:111-136` copies the master container's volume
mounts/volumes into the worker.
- `HaApplicationFactory.java:216` uses the master discovery Service as the
worker StatefulSet's `serviceName`; `:229-247` selects only masters.
**Why it matters**
The owner only needs the Kubernetes API token, but inherits the checkpoint
PVC and other master-only fields. Workers also mount the checkpoint PVC even
though only the coordinator uses checkpoint storage. This adds unnecessary
scheduling constraints and couples every role to future master changes. Worker
Pods get `subdomain=<app>-discovery`, but that Service has no worker endpoints,
so the stable worker DNS name does not resolve. HA also ignores the `mainClass`
argument (`KubernetesApplicationClusterDescriptor.java:146-151`).
**Recommendation**
Create dedicated `ObserverPodFactory` and HA `WorkerPodFactory`
implementations and compose only role-required volumes/env. Use a role-specific
headless Service, or make the discovery Service selector role-aware.
#### 8. Public APIs are widened for one provider
**Where**
- `KubernetesClient.java:131-150`: `getService`, `updateHaState`,
`deleteHaResources`, and `deleteApplication(id, uid)` are default-throwing
methods on the general client interface.
- `SeaTunnelClient.java:210-215`: new `getMasterCount()`.
**Why it matters**
An alternative `KubernetesClient` implementation or test double compiles but
throws `UnsupportedOperationException` at runtime on HA. `getMasterCount()`
counts non-lite members; in normal standalone/cluster mode workers are not
lite, so it returns all members, not masters. The name and contract only hold
for Application HA.
**Recommendation**
Move HA operations to a separate `HaApplicationClient`/capability interface.
Rename `getMasterCount` to `getDataMemberCount` or move it back to the HA
launcher.
#### 9. HA lifecycle state is scattered across four classes
**Where**
- `HaApplicationState.java:29-63`: raw string constants and a hardcoded
`terminal()`.
- `HaApplicationFactory.java:190-198`: seeds the annotations.
- `SeatunnelKubernetesHaApplication.java:313-419`: transitions and CAS.
- `KubernetesApplicationClient.java:60-89`: reads them.
**Why it matters**
Adding a phase such as `SUBMITTING` or `CANCELING` requires edits in four
places. Unrecognized values silently become `UNKNOWN`, and there is no single
transition guard.
**Recommendation**
Add a typed `HaApplicationPhase` enum and a single `HaApplicationLifecycle`
class that owns reads, CAS transitions, and terminal-state detection; have the
client, factory, and CLI delegate to it.
#### 10. Worker mode is selected by argument count
**Where**
- `SeatunnelKubernetesApplicationWorker.java:44-56`
`args.length == 4` selects HA, while the error message still documents only
three arguments. A future fourth positional argument would silently change the
execution mode.
**Recommendation**
Use an explicit mode/role argument instead of arity-based mode selection.
#### 11. Documentation and diff hygiene
- `docs/en/getting-started/application/kubernetes/commands.md:56` and
`docs/zh/.../commands.md:50` say single-master and HA `cancel` both use a UID
precondition. The code only does so for HA
(`KubernetesApplicationClient.java:131-135`); single-master still calls
`deleteApplication(id)`. Fix the docs or implement UID-safe deletion for
single-master.
- `KubernetesApplicationConfig.java:461-467` adds a pointless local
`resolved` variable, and `:743` adds an unrelated blank line. Remove this noise
so the diff remains traceable.
---
### Suggested convergence order
1. Land #12942, rebase, and remove the connector diff.
2. Extract a shared `ApplicationRuntime`/`ApplicationLifecycle` and keep the
`ApplicationResourceManager` contract for HA.
3. Replace `applicationHaEnabled` with a coordinator-only recovery policy
and remove the HA branches from `SubPlan`, `JobMaster`, and `CheckpointManager`.
4. Make checkpoint recovery a capability with submission-time validation,
and consolidate the storage scan.
5. Replace role clone-and-mutate with dedicated factories and role-specific
Services.
6. Clean up the public APIs, docs, and diff noise.
--
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]
