Skip to content

[SPARK-45777][CORE] Support spark.test.appId in LocalSchedulerBackend#43645

Closed
dongjoon-hyun wants to merge 2 commits intoapache:masterfrom
dongjoon-hyun:SPARK-45777
Closed

[SPARK-45777][CORE] Support spark.test.appId in LocalSchedulerBackend#43645
dongjoon-hyun wants to merge 2 commits intoapache:masterfrom
dongjoon-hyun:SPARK-45777

Conversation

@dongjoon-hyun
Copy link
Member

@dongjoon-hyun dongjoon-hyun commented Nov 3, 2023

What changes were proposed in this pull request?

This PR aims to support spark.test.appId in LocalSchedulerBackend like the following.

$ bin/spark-shell --driver-java-options="-Dspark.test.appId=test-app-2023"
...
Spark context available as 'sc' (master = local[*], app id = test-app-2023).
$ bin/spark-shell -c spark.test.appId=test-app-2026 -c spark.eventLog.enabled=true -c spark.eventLog.dir=/Users/dongjoon/data/history
...
Spark context available as 'sc' (master = local[*], app id = test-app-2026).

Why are the changes needed?

Like the other spark.test.* configurations, this enables the developers control the appId in LocalSchedulerBackend.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Manual.

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

No.

@github-actions github-actions bot added the CORE label Nov 3, 2023
@dongjoon-hyun
Copy link
Member Author

When you have some time, could you review this test conf PR, too, @LuciferYang ?

Copy link
Contributor

@LuciferYang LuciferYang 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

extends SchedulerBackend with ExecutorBackend with Logging {

private val appId = "local-" + System.currentTimeMillis
private val appId = sys.props.getOrElse("spark.test.appId", "local-" + System.currentTimeMillis)
Copy link
Member

Choose a reason for hiding this comment

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

Is there a particular reason to retrieve this from the system properties instead of the SparkConf?

Copy link
Member Author

Choose a reason for hiding this comment

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

Although I did to avoid SparkConf(false) case, in my use case, I always use as environment variables and it will be okay to load from SparkConf. Do you want me to change, @yaooqinn ?

Copy link
Member

Choose a reason for hiding this comment

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

From my perspective, it looks better to read from SparkConf if it's available in this case.

@dongjoon-hyun
Copy link
Member Author

I addressed your comment, @yaooqinn . Could you review this once more?

@dongjoon-hyun
Copy link
Member Author

Also, thank you for review and approval, @LuciferYang .

Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the updates

@dongjoon-hyun
Copy link
Member Author

Thank you so much! I verified manually this test conf and updated the PR description.

Merged to master!

Copy link
Contributor

@beliefer beliefer left a comment

Choose a reason for hiding this comment

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

LGTM later.

@dongjoon-hyun dongjoon-hyun deleted the SPARK-45777 branch November 4, 2023 08:00
szehon-ho pushed a commit to szehon-ho/spark that referenced this pull request Feb 7, 2024
…end`

### What changes were proposed in this pull request?

This PR aims to support `spark.test.appId` in `LocalSchedulerBackend` like the following.

```
$ bin/spark-shell --driver-java-options="-Dspark.test.appId=test-app-2023"
...
Spark context available as 'sc' (master = local[*], app id = test-app-2023).
```

```
$ bin/spark-shell -c spark.test.appId=test-app-2026 -c spark.eventLog.enabled=true -c spark.eventLog.dir=/Users/dongjoon/data/history
...
Spark context available as 'sc' (master = local[*], app id = test-app-2026).
```

### Why are the changes needed?

Like the other `spark.test.*` configurations, this enables the developers control the appId in `LocalSchedulerBackend`.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Manual.

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

No.

Closes apache#43645 from dongjoon-hyun/SPARK-45777.

Authored-by: Dongjoon Hyun <dhyun@apple.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants