Skip to content

[Fix-16617][Seatunnel] Use master option for engine deploy mode#18315

Open
JinRudy wants to merge 1 commit into
apache:devfrom
JinRudy:fix/seatunnel-master-option
Open

[Fix-16617][Seatunnel] Use master option for engine deploy mode#18315
JinRudy wants to merge 1 commit into
apache:devfrom
JinRudy:fix/seatunnel-master-option

Conversation

@JinRudy

@JinRudy JinRudy commented Jun 2, 2026 •
edited
Loading

Copy link
Copy Markdown

Was this PR generated or assisted by AI?

YES. OpenAI Codex assisted with the change and verification.

Purpose of the pull request

Close #16617.

SeaTunnel 2.3.7 still uses --deploy-mode for the Spark and Flink starters. The deprecation warning reported in the issue comes from the seatunnel.sh client path, where -e / --deploy-mode is deprecated in favor of -m / --master.

This PR keeps the change scoped to the SeaTunnel engine task command generated for seatunnel.sh.

Brief change log

  • Use --master for the SeaTunnel engine task deploy mode argument.

Verify this pull request

  • git diff --check
  • JAVA_HOME=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@17/bin:$PATH ./mvnw -pl dolphinscheduler-task-plugin/dolphinscheduler-task-seatunnel -am -Dtest=SeatunnelTaskTest,SeatunnelParametersTest -Dsurefire.failIfNoTests=false -Dsurefire.failIfNoSpecifiedTests=false -Dspotless.skip=true -Danalyze.skip=true -Djacoco.skip=true -DskipDepCheck=true test
  • JAVA_HOME=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@17/bin:$PATH ./mvnw -pl dolphinscheduler-task-plugin/dolphinscheduler-task-seatunnel -DskipTests -Djacoco.skip=true -DskipDepCheck=true spotless:check

Pull Request Notice

This pull request does not contain incompatible changes.

@boring-cyborg

boring-cyborg Bot commented Jun 2, 2026

Copy link
Copy Markdown

Thanks for opening this pull request! Please check out our contributing guidelines. (https://github.com/apache/dolphinscheduler/blob/dev/docs/docs/en/contribute/join/pull-request.md)

SbloodyS
SbloodyS reviewed Jun 3, 2026
View reviewed changes
Comment thread ...rc/main/java/org/apache/dolphinscheduler/plugin/task/seatunnel/self/SeatunnelEngineTask.java
public List<String> buildOptions() throws Exception {
List<String> args = super.buildOptions();
if (!Objects.isNull(seatunnelParameters.getDeployMode())) {
args.add(Constants.DEPLOY_MODE_OPTIONS);

@SbloodyS SbloodyS Jun 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should remove Constants.DEPLOY_MODE_OPTIONS since it's useless.

@JinRudy JinRudy Jun 3, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 935c4b6 by removing Constants.DEPLOY_MODE_OPTIONS and inlining the Spark task --deploy-mode argument to preserve the existing Spark command behavior.

@SbloodyS SbloodyS Jun 9, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You remove the usage of Constants.DEPLOY_MODE_OPTIONS. But it didn't modify its test and remove this constant.

@SbloodyS SbloodyS added first time contributor First-time contributor bug Something isn't working labels Jun 3, 2026
@SbloodyS SbloodyS added this to the 3.5.0 milestone Jun 3, 2026
SbloodyS
SbloodyS requested changes Jun 5, 2026
View reviewed changes

@SbloodyS SbloodyS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please using english in commit messages.

Comment thread ...rc/main/java/org/apache/dolphinscheduler/plugin/task/seatunnel/spark/SeatunnelSparkTask.java Outdated
public List<String> buildOptions() throws Exception {
List<String> args = super.buildOptions();
args.add(DEPLOY_MODE_OPTIONS);
args.add("--deploy-mode");

@SbloodyS SbloodyS Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the related tests.

@JinRudy JinRudy Jun 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated, thanks. I removed the test change and rewrote the branch with a single English commit.

@SbloodyS SbloodyS Jun 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still unaddressed.

@JinRudy JinRudy Jun 5, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated. I reverted the Spark/constant cleanup and kept this scoped to the SeaTunnel engine path; the PR now changes only SeatunnelEngineTask.

@SbloodyS SbloodyS Jun 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You also need to fix the related tests.

@sonarqubecloud

sonarqubecloud Bot commented Jun 5, 2026

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 60%)

See analysis details on SonarQube Cloud

@JinRudy JinRudy force-pushed the fix/seatunnel-master-option branch from 4152d48 to adbc2d8 Compare June 5, 2026 03:03
@github-actions github-actions Bot removed the test label Jun 5, 2026
@JinRudy JinRudy force-pushed the fix/seatunnel-master-option branch 2 times, most recently from 3b89bd5 to 634a73f Compare June 5, 2026 08:16
@JinRudy JinRudy force-pushed the fix/seatunnel-master-option branch from 634a73f to cf1d21d Compare June 9, 2026 08:50
@JinRudy

JinRudy commented Jun 9, 2026

Copy link
Copy Markdown
Author

Hi @SbloodyS, thanks for the follow-up. I added SeatunnelEngineTaskTest covering the engine buildOptions path:

  • buildOptionsUsesMasterForDeployMode — asserts --master cluster is emitted when deployMode is set, exercising the changed line.
  • buildOptionsOmitsMasterWhenDeployModeMissing — guards the existing null-deploy-mode behavior.
  • buildOptionsAppendsOthersWhenPresent — covers the existing others argument path together with the new flag.

I also rebased the branch onto the latest dev so the PR is no longer behind the base branch.

Local verification:

./mvnw -pl dolphinscheduler-task-plugin/dolphinscheduler-task-seatunnel -am test -Dtest=SeatunnelEngineTaskTest
# Tests run: 3, Failures: 0, Errors: 0, Skipped: 0
./mvnw -pl dolphinscheduler-task-plugin/dolphinscheduler-task-seatunnel spotless:apply

Please take another look when you have a chance.

@github-actions github-actions Bot added the test label Jun 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@SbloodyS SbloodyS SbloodyS requested changes

@caishunfeng caishunfeng Awaiting requested review from caishunfeng caishunfeng is a code owner

@Gallardot Gallardot Awaiting requested review from Gallardot Gallardot is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@JinRudy JinRudy

Labels

backend bug Something isn't working first time contributor First-time contributor test

Projects

None yet

Milestone

3.5.0

Development

Successfully merging this pull request may close these issues.

[Bug] [seatunnel task] run the seatunnel task ,report "-e and --deploy-mode deprecated in 2.3.1, please use -m and --master instead of it"

2 participants

@JinRudy @SbloodyS