[sharpca]

Purpose of this pull request

Fixes #13080.

The starters convert enum-valued command line arguments with String#toUpperCase() / String#toLowerCase() without a locale before matching them against the enum constants. Under a Turkish or Azerbaijani default locale those calls produce dotted capital I characters (client -> CLİENT, sink -> SİNK), so the values no longer match any enum constant.

As a result, with -Duser.language=tr -Duser.country=TR:

• the Spark starter rejects --deploy-mode client with No enum constant ...DeployMode.CLİENT;
• the Flink starter rejects --deploy-mode run-application and --target yarn-session / kubernetes-application;
• seatunnel-connector.sh -pt sink and the metadata export -pt sink report that the plugin type is not supported, although sink was passed;
• DeployMode.from(String) returns an empty Optional for CLIENT and throws a NullPointerException for null.

This change converts the argument with Locale.ROOT in all of those places and makes DeployMode.from locale independent and null safe.

The null correction is not limited to Turkish or Azerbaijani: DeployMode.from(null) now returns Optional.empty() on every JVM, where it previously threw. That is a deliberate contract fix in the shared helper, separate from the launcher converters, which keep their existing throwing behaviour for invalid values.

DeployMode.from now documents that contract in Javadoc, and the three new regression test classes name the production behaviour they protect.

Does this PR introduce any user-facing change?

Yes, in two ways:

• Only for JVMs whose default locale is Turkish or Azerbaijani: the CLI arguments above are now parsed the same way as on any other locale.
• For every locale: DeployMode.from(null) returns an empty Optional instead of throwing NullPointerException.

Nothing else changes for the default locale or for valid/invalid argument handling.

The ten touched files are kept as LF, so the diff no longer contains an unrelated line-ending rewrite.

How was this patch tested?

Unit tests are added:

• DeployModeTest covers DeployMode.from for valid values, an unknown value, null, and tr-TR.
• FlinkCommandArgsConverterTest covers the Flink deploy mode and master target converters, including run-application, yarn-session and kubernetes-application under tr-TR.
• SparkCommandArgsConverterTest covers the Spark deploy mode converter under tr-TR.
• ConnectorCheckCommandArgsTest#testPluginTypeArgumentIsLocaleIndependent parses -pt sink under tr-TR and also checks the metadata export converter.

All four capture the base, DISPLAY and FORMAT locale defaults, restore all three in finally, and carry @ResourceLock("java.util.Locale").

mvn -pl seatunnel-common,seatunnel-core/seatunnel-flink-starter/seatunnel-flink-starter-common,\
seatunnel-core/seatunnel-spark-starter/seatunnel-spark-starter-common,seatunnel-core/seatunnel-starter \
  -Dtest='DeployModeTest,FlinkCommandArgsConverterTest,SparkCommandArgsConverterTest,ConnectorCheckCommandArgsTest' test

Result: 9 tests, 0 failures. spotless:check passes, and the committed blobs are LF, so git diff --check is clean.

DeployModeTest was also run against the unpatched source to confirm that it fails under tr-TR (expected: <Optional[CLIENT]> but was: <Optional.empty>).

Check list

• If any new Jar binary package adding in your PR, please add License Notice according
  New License Guide
• If necessary, please update the documentation to describe the new feature. https://github.com/apache/seatunnel/tree/dev/docs
• If necessary, please update incompatible-changes.md to describe the incompatibility caused by this PR.
• If you are contributing the connector code, please check that the following files are updated:
  1. Update plugin-mapping.properties and add new connector information in it
  2. Update the pom file of seatunnel-dist
  3. Add ci label in label-scope-conf
  4. Add e2e testcase in seatunnel-e2e
  5. Update connector plugin_config

[DanielLeens]

What Problem Does This PR Solve?

User pain point: On a Turkish/Azerbaijani JVM, client and sink can uppercase to names containing a dotted I, so legitimate starter arguments fail enum lookup.

Fix approach: Use Locale.ROOT in the actual Flink/Spark/native argument converters and shared DeployMode lookup; make DeployMode.from(null) return Optional.empty.

One-sentence summary: --deploy-mode client and -pt sink should mean the same thing regardless of the launcher's default language.

Thanks for contributing this focused correction to Apache SeaTunnel. I reviewed all ten changed files at 07143665d851928d380ee906cf489083061fcce0, including the substantial line-ending churn, the argument-parser entrypoints and the new tests. The runtime direction is sound; the static whitespace check and short contract comments below need attention before submission is complete.

1. Code Change Review

1.1 Core Logic Analysis

Before:

DeployMode.valueOf(value.toUpperCase());
NAME_MAP.get(deployMode.toLowerCase());

After:

DeployMode.valueOf(value.toUpperCase(Locale.ROOT));
if (deployMode == null) {
    return Optional.empty();
}
NAME_MAP.get(deployMode.toLowerCase(Locale.ROOT));

Key findings:

• Real starter parsing reaches the changed nested converters through JCommander annotations; this is a normal launch-path fix.
• Flink's existing hyphen-to-underscore conversion and allowed-mode/target lists are unchanged, as are Spark's mode restrictions.
• Repository-wide search found no production caller of DeployMode.from; its null/uppercase correction is a shared API-helper fix, separate from the actual launcher converter paths.
• Native target conversion currently accepts LOCAL/CLUSTER, neither of which contains I; that particular ROOT change is defensive consistency, not another demonstrated Turkish startup failure.

Complete affected launch paths:

Flink AbstractFlinkStarter / AbstractSeaTunnelFlink
 -> CommandLineUtils.parse(new FlinkCommandArgs)
 -> JCommander annotated --deploy-mode / --target converters
 -> Locale.ROOT case fold -> existing '-'/'_' translation -> enum/list validation
 -> existing Flink submission flow

SparkStarter / SeaTunnelSpark
 -> CommandLineUtils.parse(new SparkCommandArgs)
 -> SparkDeployModeConverter -> Locale.ROOT -> CLIENT/CLUSTER validation
 -> existing Spark submission flow

SeaTunnelConnector.main [seatunnel-core/seatunnel-starter/.../SeaTunnelConnector.java:32]
 -> CommandLineUtils.parse(new ConnectorCheckCommandArgs)
 -> -pt SeaTunnelPluginTypeConverter -> Locale.ROOT -> PluginType
 -> existing connector command

SeaTunnelMetadataExporter.main [same module/.../SeaTunnelMetadataExporter.java:40]
 -> CommandLineUtils.parse(new MetadataExportCommandArgs)
 -> -pt PluginTypeConverter -> Locale.ROOT -> PluginType
 -> existing metadata export command

CommandLineUtils.java:85-105 retains the same JCommander parse, unknown-option and help/usage handling. No job checkpoint, recovery state, queue/RPC serializer or runtime callback is modified by this PR.

1.2 Compatibility Impact

Fully compatible for valid existing inputs. Option names, defaults, enum values, accepted mode lists, public signatures, submission protocol and persisted state remain unchanged. A deliberate correction makes null return an empty Optional instead of throwing in the shared helper; that applies to every locale, so please clarify the PR description's “only Turkish/Azerbaijani” statement as part of Issue 2. No historical job/config migration is needed for the valid CLI input correction.

1.3 Performance and Side Effects

The change adds an explicit Locale argument to the same startup string conversions. No new per-row work, threads, locks, buffers, timers, retries, network calls or ownership changes occur. CPU/memory/GC costs remain at argument parsing. The new tests mutate JVM-global state, with the limited isolation concern in Issue 3.

1.4 Error Handling and Logging

Invalid enum/mode handling and existing error text are retained; no exception swallowing or sensitive logging is introduced. The shared null helper returns empty intentionally. I found no additional functional blocker in the converter logic.

Issue 1: Remove the CRLF rewrite and pass the static whitespace check

• Location: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/DeployMode.java:1, and the other nine changed files.
• Problem: All ten head blobs contain CRLF line endings. The raw diff is +1042/-831 for a much smaller semantic change, and git diff --check upstream/dev...07143665d851928d380ee906cf489083061fcce0 exits 2 with 1042 trailing-whitespace diagnostics. The first is DeployMode.java:1. Ignoring end-of-line whitespace exposes the narrow intended patch, but does not make the submitted whitespace check pass.
• Risk: The patch fails the observed static check and obscures review/blame with unrelated line-ending changes. This is a delivery/formatting issue, not a claim of a Java runtime failure or an independently observed Spotless failure.
• Best improvement / location: Preserve repository line endings and run the normal affected-module formatter, keeping the diff to the intended conversions, null branch and tests. No shared production redesign is needed.
• Severity: Medium; blocking static-check requirement.
• Raised by another reviewer: No; the collected review/inline histories were empty.

Issue 2: Document the shared helper's null/case contract and new regression classes

• Location: seatunnel-common/src/main/java/org/apache/seatunnel/common/config/DeployMode.java:50; new DeployModeTest.java:26, FlinkCommandArgsConverterTest.java:28, SparkCommandArgsConverterTest.java:27.
• Problem: The changed public lookup has no Javadoc explaining null/unknown behavior and locale-neutral matching; the three new regression classes have no class Javadoc identifying the production behavior they protect.
• Risk: Callers cannot distinguish the helper's empty-Optional contract from the launchers' throwing converters. The description also understates that the null behavior changes on every locale.
• Best improvement / location: Add concise contracts at the helper and three test classes, and clarify the null correction in the PR text. This is a small documentation-only addition alongside the existing focused implementation; there is no need to expand the public API or add more test classes.
• Severity: Medium; blocking new-core/API comment requirement.
• Raised by another reviewer: No.

Issue 3: Restore category-specific locale defaults in the new tests

• Location: DeployModeTest.java:39-48, FlinkCommandArgsConverterTest.java:52-72, SparkCommandArgsConverterTest.java:41-49, ConnectorCheckCommandArgsTest.java:52-67.
• Problem: Each test remembers only Locale.getDefault. Locale.setDefault also changes DISPLAY and FORMAT defaults, so the finally block loses any initially distinct category defaults. Global mutation is also unguarded if same-JVM parallel testing is later enabled.
• Risk: Shared test-state pollution or environment-dependent behavior. I found no parallel JUnit execution enabled for these modules; I am not claiming a currently reproduced race.
• Best improvement / location: Capture all three defaults and restore base, DISPLAY and FORMAT in finally; use the project's shared locale resource lock if parallel execution is supported. Keep this in the existing tests, not production locale settings.
• Severity: Medium; non-blocking under the observed default execution configuration.
• Raised by another reviewer: No.

2. Code Quality Assessment

2.1 Coding Standards

Imports are explicit and new tests have ASF headers. Issues 1 and 2 cover the actual whitespace failure and short documentation gaps. The original enum lists and error branches are preserved; no unrelated dependency/configuration changes were added.

2.2 Test Coverage and Stability

The assertions test concrete parsed enum values; Spark's existing mode restriction is checked with an invalid mode. The connector test goes through CommandLineUtils.parse for -pt sink. Flink/Spark test the actual converter implementations; metadata export tests the converter directly rather than launching a subprocess. These tests do not establish external Flink/Spark execution, which is unchanged by the patch.

Stability rating: Risk present, non-blocking, due to Issue 3. All added assertions are synchronous and use fixed values, with finally cleanup. No sleeps, asynchronous result waits, ports, containers or order-dependent collections are introduced. The existing parser/launcher tests remain relevant CI evidence; no local execution is claimed.

Local analysis results: fetched the PR and checked out independent seatunnel-review-13081. Read the complete raw changed-file set and full end-of-line-normalized diff, and inspected exact-head parser entrypoints, annotations, converter restrictions and shared-helper callers. Raw diff: ten files, +1042/-831. All ten files contain CRLF. git diff --check FAILED (exit 2, 1042 whitespace diagnostics); source search for production DeployMode.from calls returned no matches. No Maven build, UT/IT/E2E, Docker, service or job execution occurred. No claim is made that Spotless was run or failed.

2.3 Documentation

No new command option or setup step requires a bilingual user-guide rewrite. Explain the null API behavior in Javadoc/PR text (Issue 2). Valid CLI spellings/defaults stay unchanged; no incompatible-changes entry is needed for accepting those already documented spellings consistently.

3. Architectural Soundness

3.1 Solution and Best Change Location

This is a targeted parsing fix at the consumers of locale-neutral enum tokens. That is the best boundary: changing the global JVM locale or each launcher script would affect unrelated libraries or duplicate the correction. The smallest safe implementation and the best architectural implementation coincide; only formatting/comments need to be added to close delivery.

3.2 Maintainability

Explicit ROOT folding keeps the convention visible. Retain the existing allowed-mode checks rather than routing every converter through a broader helper with different null/error behavior.

3.3 Extensibility

Future enum-valued argument converters can follow this local convention without a shared parser or lifecycle change.

3.4 Historical Compatibility

Existing valid command lines, serialized job/config shapes and runtime ownership remain compatible. No claim of independent launch, upgrade or checkpoint acceptance is made.

4. Issue Summary

Number	Issue	Location	Severity
1	CRLF rewrite fails git diff --check	All ten changed files, first DeployMode.java:1	Medium, blocking
2	Document helper contract and new regression classes	DeployMode.java:50 and three test-class declarations	Medium, blocking
3	Preserve category-specific locale defaults in tests	Four locale-mutating test blocks	Medium, non-blocking

Completeness check: all ten files, meaningful before/after changes and mechanical line-ending churn, actual launcher/parser callers, valid/invalid argument behavior, compatibility/defaults, startup/runtime boundary, tests/stability, documentation and CI/merge facts were covered before this consolidated list.

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers — must be fixed: Issue 1's static whitespace check; Issue 2's concise helper/test contracts; required current-head CI and effective maintainer approval.
2. Recommended fixes — non-blocking: Issue 3's full locale restoration and parallel-test isolation where applicable.

Overall assessment: The functional correction is narrow and at the right layer. Please keep the follow-up to formatting, concise contracts and test-state restoration, preserving the useful regression assertions. No other maintainer approval/change request was present in the collected history. The exact-head PR signal is action_required with no check rollup; a maintainer should inspect its approval gate. This does not establish that fork Actions is disabled, and a base sync is not a remedy for that workflow gate. Daniel has READ permission; this COMMENT does not supply effective write-capable approval.

Snapshot 2026-11-01T11:14:32.361817+00:00: head 07143665d851928d380ee906cf489083061fcce0, base dev; compare diverged, ahead 1 / behind 3; merge blocked / UNKNOWN, review decision REVIEW_REQUIRED. Drift is recorded separately; it does not itself request synchronization.

[sharpca]

All three issues are addressed in 3dc5920e.

Issue 1 — CRLF / git diff --check (blocking) — the ten files are LF now:

• Net diff against dev for the same ten files went from +1042/-831 to +268/-7.
• The blob, not just the working tree: DeployMode.java at 3dc5920e is 2306 bytes with 0 CR and 66 LF; the compare patch for this head contains no CR.
• git diff --check is clean for the new commit.

Worth recording why it happened and why the naive fix is not enough: the repository has no *.java rule in .gitattributes, and core.autocrlf is true on this Windows checkout, so spotless:apply writes CRLF back into the working tree even after normalisation. I applied the formatter first, normalised afterwards, and committed through the index, which is why the committed blobs are LF.

Issue 2 — contracts (blocking) — DeployMode.from now documents that it folds with Locale.ROOT and returns an empty Optional for null and unknown tokens, and the three regression classes have class Javadoc naming the production behaviour they protect. The description now states explicitly that the null correction applies on every locale, not only Turkish/Azerbaijani, and that the launcher converters keep their existing throwing behaviour.

Issue 3 — locale isolation — all four locale-mutating tests capture the base, DISPLAY and FORMAT defaults, restore all three in finally, and carry @ResourceLock("java.util.Locale").

Verification (JDK 17, the four affected modules): DeployModeTest 2, FlinkCommandArgsConverterTest 3, SparkCommandArgsConverterTest 2, ConnectorCheckCommandArgsTest 2 — 9 tests, 0 failures. spotless:check passes.

Still open on my side: current-head CI is action_required, so none of this has executed in Actions; that gate needs a maintainer.

[DanielLeens]

Thanks for the update! I re-read all ten current changed files, parser entrypoints and converter restrictions at 3dc5920ed429c279385132a6c9bb8df812d2f683. All three earlier items are resolved in this source version. This single review also answers the follow-up; no new blocker was found.

What this PR solves

User pain: valid client, sink and Flink target/mode tokens can fail enum parsing under Turkish/Azerbaijani defaults. Approach: explicitly fold machine tokens with Locale.ROOT in shared lookup and real launcher converters. One sentence/example: -pt sink should resolve to SINK on every machine, not fail because uppercasing produced a dotted I.

1. Code Change Review

1.1 Core Logic Analysis

Actual changes: DeployMode.from:53-65 adds null handling and ROOT lowercase; FlinkCommandArgs:107-133 uses ROOT uppercase while retaining hyphen conversion and allowed lists; SparkCommandArgs:80 uses ROOT with existing CLIENT/CLUSTER restriction; native ClientCommandArgs:335 and both connector/metadata plugin converters use ROOT. Before/after:

// before
NAME_MAP.get(deployMode.toLowerCase());
DeployMode.valueOf(value.toUpperCase());
// after
if (deployMode == null) return Optional.empty();
NAME_MAP.get(deployMode.toLowerCase(Locale.ROOT));
DeployMode.valueOf(value.toUpperCase(Locale.ROOT));

Complete affected chain:

Flink AbstractFlinkStarter / AbstractSeaTunnelFlink
 -> CommandLineUtils.parse -> JCommander annotated converters
 -> ROOT folding + existing '-'/'_' translation + mode/target allowlist
 -> unchanged Flink submission
SparkStarter / SeaTunnelSpark -> parse -> SparkDeployModeConverter
 -> ROOT folding + CLIENT/CLUSTER validation -> unchanged Spark submission
SeaTunnelConnector.main:32 -> parse ConnectorCheckCommandArgs
 -> -pt SeaTunnelPluginTypeConverter -> PluginType -> connector command
SeaTunnelMetadataExporter.main:40 -> parse MetadataExportCommandArgs
 -> -pt PluginTypeConverter -> PluginType -> metadata command

Key findings: normal argument parsing reaches the fix; the shared Optional helper is separate from throwing launcher converters; native LOCAL/CLUSTER folding is defensive consistency rather than a demonstrated I-related failure; runtime checkpoint/state/async processing is unaffected. The current committed diff is ten files +268/-7 and git diff --check exits 0, replacing the earlier CRLF-heavy failing snapshot. Raw source contains the shared null/unknown/case contract and the three test-class Javadocs. All four locale tests now restore base, DISPLAY and FORMAT defaults in finally and use the same resource lock.

Evidence: independent review branch seatunnel-review-13081, head 3dc5920ed429c279385132a6c9bb8df812d2f683. Local Git fetch/diff and source/caller searches produced the complete changed-file evidence; I ran no local build, test, container or job.

1.2 Compatibility Impact

Fully compatible for intended existing tokens and unchanged API/config/state contracts. No existing option/default, enum values, signature, protocol or checkpoint shape changes. DeployMode.from(null) now deliberately returns empty on every locale; launcher converters retain their existing throwing behavior and allowed lists. This is documented rather than hidden as a Turkish-only behavior.

1.3 Performance / Side Effects

Same startup/metadata-time string operations and linear complexity; no new worker, thread, lock, retry loop, network operation or per-row allocation. No data plane or transaction ownership changes.

1.4 Error Handling and Logging

Existing invalid-mode/type exceptions and messages remain. Shared null lookup returns empty intentionally; no exception swallowing, new logging or credential exposure. No blocking source issue found. Earlier Issue 1 whitespace, Issue 2 contracts and Issue 3 test isolation are closed on this head.

2. Code Quality Assessment

2.1 Coding Standards

Explicit imports, focused changes and ASF headers are present. The helper and new regression class contracts are now documented. CRLF churn is absent from the committed diff and static whitespace check passes.

2.2 Test Coverage and Test Stability

Stability rating: Stable. DeployModeTest, FlinkCommandArgsConverterTest, SparkCommandArgsConverterTest and ConnectorCheckCommandArgsTest use synchronous exact enum assertions, try/finally restoration of all locale defaults and a shared locale resource lock. Real JCommander parsing is exercised for connector -pt; Flink/Spark tests use their production converters. Invalid Spark modes remain checked. No sleeps, asynchronous waits or port/container assumptions were added. Contributor-reported local test/Spotless results are not execution by this review; no local tests or job were run.

2.3 Documentation Updates

No new user option or setup is introduced. The public null/case behavior is documented in DeployMode and the updated PR description. Existing valid command spellings and help behavior remain. No EN/ZH setup rewrite or incompatible-change entry is required.

3. Architectural Soundness

3.1 Elegance

A precise machine-token parsing correction at its consumers; changing the JVM default in production would affect unrelated libraries and is unnecessary.

3.2 Maintainability

Explicit ROOT selection keeps the semantic assumption visible without a new shared framework. Preserve launcher-specific error and allowlist behavior instead of unifying it with the Optional helper.

3.3 Extensibility

Future converters can use the same explicit locale convention without modifying engine or SPI behavior. No broad parser/normalizer refactor is required.

3.4 Historical-Version Compatibility

Existing API, configuration defaults and serialized job/checkpoint contracts remain. The documented null acceptance is deliberate; this review does not certify a live Flink/Spark launch.

4. Issue Summary

ID	Issue	Location	Severity
-	No open source blocker; previous 1/2/3 resolved	Current ten-file diff	-

The author supplied a per-item response; current source independently confirms LF blobs, helper/test contracts and full locale restoration. No non-Daniel review/inline finding exists to duplicate. I agree that the remaining Actions approval must be handled by a maintainer.

5. Merge Recommendation

Conclusion: can merge

1. Blockers, must fix: No source-level blocker remains; current-head required CI/workflow approval and effective maintainer review remain repository merge gates.
2. Suggested improvements, nonblocking: No new nonblocking source request. The previous locale-isolation suggestion is resolved.

The previous head's gate snapshot is superseded by this new SHA. Current Apache check rollup and fork run query are empty. The author reports an action_required PR signal; workflow approval is a maintainer action, not evidence of passing Build or disabled fork CI. Branch comparison is diverged, ahead 2 / behind 3 from dev, without reported content conflict. Drift is recorded separately; no unrelated failed-CI/sync cure is established. No empty commit, unrelated workflow patch or base-age-only sync is requested.

The small revision closes the earlier source requests and retains the focused runtime fix. I have no new source blocker; please proceed through the existing CI and maintainer gates. Daniel READ technical clearance is separate from an effective write-reviewer approval.

Daniel has READ permission. This technical conclusion does not dismiss historical requested changes or satisfy a write-reviewer gate.

Snapshot 2026-11-01T15:43:17.973267+00:00: head 3dc5920ed429c279385132a6c9bb8df812d2f683, base dev; compare diverged, ahead 2 / behind 3; merge blocked / BLOCKED, review decision REVIEW_REQUIRED. Drift is recorded separately; it does not itself request synchronization.

[sharpca]

Thanks @DanielLeens, and thanks for the correction on both points.

CI
You are right and I had it wrong: I was reading action_required on the Build check as an approval gate that a maintainer had to clear, and it is not. The build has to run in sharpca/seatunnel, that fork has no workflow runs at all, and that is why the check reports "Workflow run detection failed". So there is nothing for anyone to approve, and the ball is in my court:

1. enable Actions on the fork,
2. run the Build workflow for this branch (or push an empty commit),
3. let the PR check adopt the fork run.

I will do that for #13081 and for #13083. If the check still reports "Workflow run detection failed" after the fork run exists, I will reply here with the run id rather than assume it is a flake.

--checkpoint-history-status — agreed, and I will fix it in this PR
It is the same class of bug as the issue title, so it belongs here rather than in a follow-up. ClientExecuteCommand currently does:

status = CheckpointStatus.valueOf(
        clientCommandArgs.getCheckpointHistoryStatus().toUpperCase());

Under tr-TR, failed upper-cases to FAİLED (dotted İ, U+0130), valueOf throws, and the branch reports "Unsupported checkpoint history status" for an input that is valid everywhere else. The fix is the same shape as the rest of the issue: toUpperCase(Locale.ROOT), pulled into a small parseCheckpointStatus(String) helper so it can be asserted directly instead of through a client that needs a cluster — ClientExecuteCommand#execute builds a SeaTunnelClient, so a test that goes through execute() would be an integration test for one string conversion.

One correction to your message while I am here: CheckpointStatus has four constants, not two — IN_PROGRESS, COMPLETED, FAILED, CANCELED. The new test covers all four under tr-TR (and asserts that an unknown value still throws), so the assertion pins the whole enum rather than the two values that happen to be in use today. seatunnel-starter already has JUnit 5 tests in the same package (ApplicationExecuteCommandTest, ConnectorCheckCommandTest, ...), so no build change is needed.

On the other seatunnel-core conversions
Agreed that AbstractFlinkRuntimeEnvironment (checkpoint mode, time characteristic) reads job config rather than CLI arguments and is out of scope for this issue. I will not touch it here; if it deserves a fix it should be its own issue, since the trigger is a config file value rather than an argument a user types.

Once the fork build has run I will ping here with the result, and the detailed review can continue on #13081.

[DanielLeens]

Thanks for the quick follow-up, @sharpca! Before anything else, a sincere apology on two carryover points from my earlier reviews on this PR: I missed --checkpoint-history-status in ClientExecuteCommand when I said no other blocker remained on 3dc5920e, and I described the action_required signal as a maintainer approval gate. Both were wrong, and thank you for picking them up so cleanly.

I re-reviewed the current head cac71da1e29ecfbbf7890286dbecbd791b732bf3. The only change since my last review is the new commit cac71da1, touching ClientExecuteCommand.java and adding ClientExecuteCommandCheckpointStatusTest.java. The other ten files are byte-for-byte what I reviewed at 3dc5920e, so my earlier conclusions on those still hold.

What Problem Does This PR Solve?

User pain point: On a JVM with a Turkish or Azerbaijani default locale, valid CLI tokens such as client, sink, run-application, and now failed / in_progress, are uppercased to strings with a dotted capital I, so the enum lookup rejects them.

Fix approach: Fold machine tokens with Locale.ROOT in every CLI enum converter in the starters, and make DeployMode.from handle null and locale-independent matching. The new commit applies the same fix to the Zeta client's --checkpoint-history-status filter.

One-sentence summary: seatunnel.sh --checkpoint-history <jobId> --checkpoint-history-status failed now filters by FAILED on a tr-TR machine instead of failing with "Unsupported checkpoint history status failed".

1. Code Change Review

1.1 Core Logic Analysis

Before (ClientExecuteCommand.java on dev, around line 160):

status = CheckpointStatus.valueOf(
        clientCommandArgs.getCheckpointHistoryStatus().toUpperCase());

After (ClientExecuteCommand.java:160-163, :328-330):

status = parseCheckpointStatus(clientCommandArgs.getCheckpointHistoryStatus());

static CheckpointStatus parseCheckpointStatus(String status) {
    return CheckpointStatus.valueOf(status.toUpperCase(Locale.ROOT));
}

Runtime flow of the newly covered path:

SeaTunnelClient main -> CommandLineUtils.parse(ClientCommandArgs)
 -> --checkpoint-history-status stored as a raw String (ClientCommandArgs.java:146-148)
ClientExecuteCommand.execute()
 -> checkpointHistoryJobId != null branch (ClientExecuteCommand.java:151)
 -> null guard on the status (line 159), so the helper never sees null
 -> parseCheckpointStatus: ROOT uppercase -> CheckpointStatus.valueOf
 -> IllegalArgumentException still wrapped into CommandExecuteException
    "Unsupported checkpoint history status %s" (lines 164-170)
 -> JobClient.getCheckpointHistory(jobId, pipelineId, limit, status) unchanged

Key findings:

• The normal user path hits the change: any --checkpoint-history-status value containing i (failed, in_progress) was broken under tr-TR; completed / canceled were not affected, but are now folded consistently.
• CheckpointStatus has four constants (IN_PROGRESS, COMPLETED, FAILED, CANCELED, in seatunnel-engine-core/.../checkpoint/CheckpointStatus.java); all are covered by the new test.
• The helper is package-private and static, so it adds no public API, and it keeps the existing null guard and exception wrapping at the call site.
• Leaving AbstractFlinkRuntimeEnvironment (job-config values, not CLI arguments) out of scope is reasonable and matches the issue title.

1.2 Compatibility Impact

Fully compatible. The option name, accepted values, help text and error message are unchanged. For non-Turkish locales, toUpperCase() and toUpperCase(Locale.ROOT) give the same result for these ASCII tokens, so existing command lines behave exactly as before. No RPC payload, checkpoint state or serialized shape changes; JobClient.getCheckpointHistory receives the same enum value.

1.3 Performance and Side Effects

One string conversion at client command start-up, as before. No threads, network calls, retries or data-path work are added.

1.4 Error Handling and Logging

Unknown values still raise IllegalArgumentException, which the existing catch turns into the same CommandExecuteException. Nothing is swallowed and no logging changes. No new issues found in this area.

2. Code Quality Assessment

2.1 Coding Standards

The helper's Javadoc explains why Locale.ROOT is needed (the FAİLED example), the new test class has the ASF header and a class Javadoc that names the protected behaviour, and java.util.Locale is imported explicitly. Style matches the rest of the PR.

2.2 Test Coverage and Stability

ClientExecuteCommandCheckpointStatusTest.java:60-81 asserts exact enum results for all four constants under tr-TR (including mixed-case Completed) and that an unknown value (finished) still throws. Removing Locale.ROOT from parseCheckpointStatus makes the failed and in_progress assertions fail, so the test has real failing power.

Stability rating: Stable. Locale capture happens in @BeforeEach (lines 45-51) and restoration of base, DISPLAY and FORMAT in @AfterEach (lines 53-58), and the class carries the same @ResourceLock("java.util.Locale") as the other four locale tests. No sleeps, ports, containers or async waits. Testing the helper directly rather than through execute() (which builds a SeaTunnelClient) is the right granularity.

2.3 Documentation

docs/en/engines/zeta/user-command.md:28 and the zh counterpart already document --checkpoint-history-status; accepted values do not change, so no doc update is needed.

3. Architectural Soundness

The fix stays at the consumer of the CLI token, like the rest of the PR, instead of changing the JVM default locale or adding a shared parser. The minimal change and the best change location are the same here.

Optional, non-blocking follow-up (not needed for this PR): UsageFormatter.java:84 lowercases enum defaults for --help output with the default locale, so under tr-TR the help can print clıent (dotless i). It is display-only and still parses correctly with Locale.ROOT now, so it can be a separate small change if you want full consistency.

4. Issue Summary

ID	Issue	Location	Severity
-	No open source-level issue; the --checkpoint-history-status gap is fixed	ClientExecuteCommand.java:160-163, 328-330	-
A	Optional: help output lowercases enum defaults with the default locale	UsageFormatter.java:84	Low, non-blocking
CI	No CI run exists for this head	fork sharpca/seatunnel has no workflow runs	Blocking until it runs

5. Merge Recommendation

Conclusion: Ready to merge after fixes

1. Blockers, must fix: Only CI. The fork sharpca/seatunnel currently has no workflow runs at all, so the apache-side PR signal run (actions/runs/41660131748) ends in action_required and no Build check exists on this head. As you already planned: enable Actions on the fork, run the Build workflow for fix/cli-enum-locale, and the PR check will pick up the fork run. There is no source-level blocker.
2. Non-blocking: Item A above, if you want it.

The branch is 3 commits ahead and 8 behind dev with no content conflict (MERGEABLE); a sync is not required for this review. Once the fork Build is green, this looks good to me. Thanks again for the careful, well-documented work.

[sharpca]

Thanks for the re-review, and no apology needed — you caught your own two carryovers before I did, which is more than most reviews do.

On CI: agreed, and it is the only thing left. I have the fork build on me, not on a maintainer: sharpca/seatunnel still reports zero registered workflows (GET /repos/sharpca/seatunnel/actions/workflows → total_count: 0, .../actions/runs → 0), and gh workflow run Build --ref fix/cli-enum-locale fails with "could not find any workflows named Build". So the workflows are simply not enabled on the fork yet; once I flip that switch in the fork's Actions tab, I will run Build for fix/cli-enum-locale (and for #13083) and the PR checks should adopt those runs. I will post the run links here rather than describe them.

On Item A (UsageFormatter.java:84): you are right, and I confirmed it on dev — it is the only case conversion in that file, java.util.Locale is not even imported there, and String displayText = type.isEnum() ? def.toString().toLowerCase() : def.toString(); will print clıent for a client default under tr-TR. I agree it does not belong in this PR, which is otherwise done: I have it prepared as a one-line Locale.ROOT change plus the import and the reformat that the 100-column limit forces. I will open it as a separate small PR once this one has merged, so this PR can land as soon as the build is green.

Thanks also for confirming the four CheckpointStatus constants and that the branch does not need a sync at 3 ahead / 8 behind.

[sharpca]

CI is running now that Actions is enabled on the fork: Build / fix/cli-enum-locale (workflow_dispatch, head cac71da). I will report the result here when it finishes.

[DanielLeens]

Thanks for enabling Actions on your fork and running the build, @sharpca. I checked the run you linked: Build 41673624221 ran on the exact head cac71da1e29ecfbbf7890286dbecbd791b732bf3 and completed successfully, with 85 jobs passed and 11 skipped (including unit-test on JDK 11/17 for ubuntu and windows, the code-style, license and dependency checks, and the connector/engine IT groups).

In my last review CI was the only remaining blocker, and the head has not changed since, so I am updating my review to approve. One note for the merging maintainer: the run was started with workflow_dispatch, and the apache-side Build check for this head has not attached yet; the scheduled Update build status workflow should adopt it, otherwise the fork run above is the evidence. The UsageFormatter.java:84 help-text item stays an optional follow-up, as you planned.

Thanks again for the careful, well-documented fix.

[sharpca]

The fork build is green on cac71da: Build / fix/cli-enum-locale - 96 jobs, 0 failures, including Code style, unit-test on 11/17 x ubuntu/windows and the JDBC IT shards. Nothing else outstanding from my side on this PR.

[sharpca]

Fork Actions had been disabled at the repository level; it is enabled again and I dispatched the workflow, so the PR check now reports the fork run for the current head.

• Run: https://github.com/sharpca/seatunnel/actions/runs/41785719208 (head cac71da1) — failure
• 96 jobs; two failing: unit-test (17, ubuntu-latest) and all-connectors-it-5 (17, ubuntu-latest).
• all-connectors-it-5: the uploaded report shows TiDBCDCIT failing with Tests run: 1, Errors: 1 and com.mysql.cj.jdbc.exceptions.CommunicationsException: Communications link failure from TiDBCDCIT.startUp — the same e2e container-startup failure that [Fix][Connector-V2] Make JDBC dialect type converters locale independent #13083's run shows, on a branch with a different change set.
• unit-test (17): every step is reported as successful while the job is marked failed, and that job uploaded no report, so I cannot state its cause from the artifacts. The same job also failed on this branch's earlier commit 24c2d878 (run https://github.com/sharpca/seatunnel/actions/runs/41675888169), so it is not specific to the current head.

On --checkpoint-history-status: it is fixed in this PR at cac71da1, not deferred. ClientExecuteCommand parses it with CheckpointStatus.valueOf(status.toUpperCase(Locale.ROOT)), and ClientExecuteCommandCheckpointStatusTest covers failed and in_progress under tr-TR, restores the base, DISPLAY and FORMAT locale defaults, and carries @ResourceLock("java.util.Locale"). No separate issue is needed.

[sharpca]

Update on unit-test (17, ubuntu-latest): it is not a test failure and it is not the job timeout.

• unit-test declares timeout-minutes: 120 in backend.yml, and the job ran 06:30:57Z → 07:25:11Z, i.e. 54m14s.
• In the job record, step 4 run all modules unit test is still in_progress with a null conclusion, and Upload test reports plus the two post steps are pending. The job therefore ended while the Maven step was still executing, before anything could be uploaded.
• That job produced no report artifact (the run's only artifact belongs to all-connectors-it-5), so no test failed.
• The same job also failed on this branch's earlier commit 24c2d878 (run https://github.com/sharpca/seatunnel/actions/runs/41675888169), so it is not specific to cac71da1.

So the job was terminated runner-side mid-step, which is also why every completed step looks green while the job is red. The remaining failure in this run is the TiDBCDCIT container-startup CommunicationsException in all-connectors-it-5, unchanged from my previous comment.

[DanielLeens]

Thanks @sharpca for digging into both failures, and for confirming the --checkpoint-history-status path. I checked it at cac71da1: ClientExecuteCommand.java:329 uses CheckpointStatus.valueOf(status.toUpperCase(Locale.ROOT)), and ClientExecuteCommandCheckpointStatusTest is in the PR, so I agree no separate issue is needed.

On CI: my approval stands. It was based on run 41673624221, which completed green on this exact head, and the head has not changed since. Your read of the later run 41785719208 matches what I see. In all-connectors-it-5, TiDBCDCIT fails in startUp with a container CommunicationsException, which is environmental; this PR does not touch the TiDB CDC connector. unit-test (17) stopped mid-step on the runner side, with no test report.

The apache-side Build check currently points at the failed run 41785719208. Attempt 2 of run 41881173697 is in progress. If it finishes green, the check should switch to green on its own. Either way, nothing more is needed from you on the code. Thanks again!

[sharpca]

Green run on cac71da as well: Build 41881173697 (attempt 2) - conclusion success.

Attempt 1 failed on a single infrastructure job (all-connectors-it-5 (17), TiDBCDCIT: connection refused / server went away); re-running only that job turned the run green, so this head now has a fully green fork build, on top of the earlier green run 41673624221 that Daniel already used for the approval.

Nothing else is outstanding from my side on this PR; the apache-side Build check should adopt the fork run on its next scheduled tick.

[DanielLeens]

Thanks for the update, @sharpca. The CI fact cited in my previous reply has changed: attempt 2 of run 41881173697 completed successfully on the unchanged head cac71da1e29ecfbbf7890286dbecbd791b732bf3. My existing technical approval stands; no source change is requested.

The Apache Build check still points to the older failed run 41785719208 in the current snapshot. I should correct my earlier suggestion that it would necessarily switch on its own: the remaining step is for a maintainer to reconcile/retrigger the PR-signal/check-adoption workflow against the successful exact-head run, then confirm branch protection. Please keep the green run link rather than changing unrelated TiDB code or syncing merely to replace that stale check. This is a CI-gate clarification, not another source review.

[sharpca]

Understood, and the branch stays untouched - no TiDB changes, no dev sync, nothing that would move the head away from cac71da1e.

The green evidence for this exact head, for whoever reconciles the check:

• Fork build 41881173697, attempt 2, head cac71da1e29ecfbbf7890286dbecbd791b732bf3, conclusion success: https://github.com/sharpca/seatunnel/actions/runs/41881173697
• Attempt 1 failed only on all-connectors-it-5 (17) -> TiDBCDCIT (connection refused, then server went away); re-running that single job turned the whole run green.

I tried to retrigger the adoption side myself, and it is indeed outside my permissions:

gh workflow run update_build_status.yml --repo apache/seatunnel --ref dev
-> HTTP 403: Must have admin rights to Repository

The scheduled run keeps ticking successfully (latest at 08:34Z) but skips this PR because the Build check is already final for run 41785719208 - which matches exactly what you described. So the reconcile step needs someone with write access; I will not touch the branch in the meantime, and the run link above is the evidence to reconcile against.

For completeness on the sibling PR: #13083 is now fully green - its Build check was adopted on head b3209869 (Build: pass, Signal: pass), and it also only awaits a committer.

[DanielLeens]

Thanks @sharpca, and thanks for checking the dispatch permission. That 403 is expected for contributors.

I looked at how update_build_status.yml picks its run, and there is one step you can take without moving the head. The Apache Build check (run id 126428630380) stores run_id 41785719208 in its metadata, and the updater only follows that run. It never looks at run 41881173697, so the green run there can't be adopted on its own. What the updater does handle is a new attempt of the run it tracks: when run 41785719208 gets a new attempt, it creates a fresh Build check for that attempt and fills in the result once the attempt finishes.

So the cheapest next step is to open run https://github.com/sharpca/seatunnel/actions/runs/41785719208 in your fork and use "Re-run failed jobs". That run is on the same head cac71da1e, and only two jobs failed in it: unit-test (17), which the runner stopped mid-step, and all-connectors-it-5 (17), the TiDBCDCIT container start. Both are the environmental failures we already discussed. No commit, sync or TiDB change is needed. If either job fails again for the same environmental reason, rerunning just that job is still the right move. If that doesn't get the check moving, a committer can dispatch the workflow.

My approval on cac71da1 stands.

[sharpca]

The rerun worked exactly as you described: attempt 2 of run 41785719208 finished green (https://github.com/sharpca/seatunnel/actions/runs/41785719208), and the Build check on this PR now carries that attempt's result - Build: pass, Signal: pass.

So on cac71da both checks are green, your approval stands, and nothing is left on my side except the merge itself.

[sharpca]

Gentle ping for a write-capable review, since this has been green and technically approved for a while and nothing is pending on the author side.

State on cac71da1e: Build: pass, Signal: pass, Daniel's technical approval recorded, mergeable: MERGEABLE. The Build check was refreshed by re-running only the two environmentally failed jobs on the tracked run (attempt 2 of run 41785719208), and the branch has not moved since.

The only remaining gate is a committer review and a merge through the dev merge queue - contributors cannot enqueue it (verified: EnablePullRequestAutoMerge is denied for the author, and reviewRequests is empty because the author cannot assign a reviewer). If a maintainer needs anything else - a label, a checklist item, a rebase - tell me and I will add it.