Skip to content

Conversation

@TQJADE
Copy link
Contributor

@TQJADE TQJADE commented Jul 2, 2025

What changes were proposed in this pull request?

SparkAppDriverConf should use the value stored in effectiveSparkConf rather than pre-computed Id for creation.

Why are the changes needed?

When user set the spark.app.id in sparkConf, operator should honor this value.

Does this PR introduce any user-facing change?

no

How was this patch tested?

unit test

Was this patch authored or co-authored using generative AI tooling?

no

@TQJADE TQJADE changed the title Fix the bug when user has set the spark.app.id value [SPARK-52657] Fix the bug when user has set the spark.app.id value Jul 2, 2025
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Could you fill the PR description, @TQJADE ?

return SparkAppDriverConf.create(
effectiveSparkConf,
appId,
effectiveSparkConf.getAppId(),
Copy link
Member

@dongjoon-hyun dongjoon-hyun Jul 2, 2025

Choose a reason for hiding this comment

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

Please add a test case to SparkAppSubmissionWorkerTest.java.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, @TQJADE .

@TQJADE TQJADE requested a review from dongjoon-hyun July 2, 2025 22:25
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-52657] Fix the bug when user has set the spark.app.id value [SPARK-52657] Allow users to set spark.app.id Jul 2, 2025
@dongjoon-hyun dongjoon-hyun changed the title [SPARK-52657] Allow users to set spark.app.id [SPARK-52657] Allow spark.app.id Jul 2, 2025
Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

cc @viirya , @peter-toth , @jiangzho

@dongjoon-hyun
Copy link
Member

Thank you, @viirya and @TQJADE .
Merged to main.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants