[SPARK-42699][CONNECT] SparkConnectServer should make client and AM same exit code #40315
Conversation
|
ping @HyukjinKwon |
| /** | ||
| * Stop the underlying `SparkContext`. | ||
| * | ||
| * @since 2.0.0 | ||
| */ | ||
| def stop(): Unit = { | ||
| sparkContext.stop() | ||
| def stop(exitCode: Int = 0): Unit = { |
There was a problem hiding this comment.
@AngersZhuuuu this is a breaking change. You will need to define two stop methods, one with the status code, and one without.
HyukjinKwon
changed the title
| /** | ||
| * Stop the underlying `SparkContext`. | ||
| * | ||
| * @since 2.0.0 | ||
| */ | ||
| def stop(): Unit = { | ||
| sparkContext.stop() | ||
| def stop(exitCode: Int = 0): Unit = { |
There was a problem hiding this comment.
Can you remove the default value here?
| @@ -736,13 +736,15 @@ class SparkSession private( | |||
| } | |||
| // scalastyle:on | |||
|
|
|||
| def stop(): Unit = stop(0) | |||
|
|
|||
| /** | |||
| * Stop the underlying `SparkContext`. | |||
| * | |||
| * @since 2.0.0 | |||
There was a problem hiding this comment.
You should have the new docs for the one for exitCode with new @since
There was a problem hiding this comment.
How about current?
|
ping @HyukjinKwon |
| * | ||
| * @since 2.0.0 | ||
| */ | ||
| def stop(): Unit = { | ||
| sparkContext.stop() | ||
| def stop(): Unit = stop(0) |
There was a problem hiding this comment.
I think it is better to not change this line. This line builds on sparkContext.stop(). Even though the underlying implementation is SparkContext.stop(0), I think we'd better do not make that assumption that it will always be.
|
@amaliujia Like current? also ping @HyukjinKwon |
|
LGTM |
|
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable. |
What changes were proposed in this pull request?
Since in #35594 we support pass a exit code to AM,
when SparkConnectServer exit with -1, need pass this to AM too.
Why are the changes needed?
Keep same exit code
Does this PR introduce any user-facing change?
No
How was this patch tested?