[SPARK-38270][SQL] Spark SQL CLI's AM should keep same exit code with client side #35594

AngersZhuuuu · 2022-02-21T08:32:13Z

What changes were proposed in this pull request?

Current Spark SQL CLI alway use shutdown hook to stop SparkSQLEnv

 // Clean up after we exit
    ShutdownHookManager.addShutdownHook { () => SparkSQLEnv.stop() }

but use process ret to close client side jvm

while (line != null) {
  if (!line.startsWith("--")) {
    if (prefix.nonEmpty) {
      prefix += '\n'
     } 
    if (line.trim().endsWith(";") && !line.trim().endsWith("\\;")) {
      line = prefix + line
      ret = cli.processLine(line, true)
      prefix = ""
      currentPrompt = promptWithCurrentDB
    } else {
      prefix = prefix + line
      currentPrompt = continuedPromptWithDBSpaces
    }
  }
  line = reader.readLine(currentPrompt + "> ")
}

sessionState.close()

System.exit(ret)
}

    if (sessionState.execString != null) {
      exitCode = cli.processLine(sessionState.execString)
      System.exit(exitCode)
    }

    try {
      if (sessionState.fileName != null) {
        exitCode = cli.processFile(sessionState.fileName)
        System.exit(exitCode)
      }
    } catch {
      case e: FileNotFoundException =>
        logError(s"Could not open input file for reading. (${e.getMessage})")
        exitCode = 3
        System.exit(exitCode)
    }

This always cause client side exit code not consistent with AM.

IN this pr I prefer to pass the exit code to SparkContext.stop() method to pass a clear client side status to AM side in client mode.

In this pr, I add a new stop method in SchedulerBackend

def stop(exitCode: Int): Unit = stop()

So we don't need to implement it for all kinds of scheduler backend.
In this pr, we only handle the case me meet for YarnClientSchedulerBackend, then we can only implement stop(exitCode: Int) for this class, then for yarn client mode, it can work now.

I think this can benefit many similar case in future.

Why are the changes needed?

Keep client side status consistent with AM side

Does this PR introduce any user-facing change?

With this pr, client side status will be same with am side

How was this patch tested?

MT

… client side

LuciferYang · 2022-02-21T14:26:16Z

IN this pr I prefer to pass the exit code to SparkContext.stop() method to pass a clear cliet side status to AM side in client mode. And I think this can benefit many similar case in future.

cliet -> client

This reverts commit 4b3ee98.

This reverts commit 3ac12c0.

This reverts commit 69bbafa.

This reverts commit 1b4ee13.

This reverts commit 23f4f85.

AngersZhuuuu · 2022-02-22T07:36:57Z

IN this pr I prefer to pass the exit code to SparkContext.stop() method to pass a clear cliet side status to AM side in client mode. And I think this can benefit many similar case in future.

cliet -> client

updated

AngersZhuuuu · 2022-02-22T07:38:09Z

gentle ping @cloud-fan @tgravescs @mridulm

core/src/main/scala/org/apache/spark/SparkContext.scala

cloud-fan · 2022-02-22T07:46:57Z

cc @bogdanghit @yaooqinn

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala

...e-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala

LuciferYang

LGTM +1

AngersZhuuuu · 2022-02-28T07:19:04Z

gentle ping @cloud-fan @bogdanghit

bogdanghit · 2022-02-28T23:55:54Z

@AngersZhuuuu does this address the CliSuite flakiness I reported a while back?

AngersZhuuuu · 2022-03-01T03:32:29Z

@AngersZhuuuu does this address the CliSuite flakiness I reported a while back?

Can you provide the link？ Literally, it should be irrelevant

AngersZhuuuu · 2022-03-29T03:15:10Z

gentle ping @bogdanghit

github-actions · 2022-09-27T00:28:09Z

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.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!

cloud-fan · 2022-09-28T02:55:55Z

@AngersZhuuuu can you rebase? We should merge this PR.

yaooqinn · 2022-09-28T03:21:21Z

+1, and sorry for not merging it after my approval

AngersZhuuuu · 2022-09-28T08:23:52Z

@AngersZhuuuu can you rebase? We should merge this PR.

Done

AngersZhuuuu · 2022-10-28T03:06:24Z

ping @cloud-fan @yaooqinn @LuciferYang

cloud-fan · 2022-11-01T07:25:38Z

thanks, merging to master!

…tElementNames in Mima for Scala 2.13 ### What changes were proposed in this pull request? This PR is a followup of #35594 that recovers Mima compatibility test for Scala 2.13. ### Why are the changes needed? To fix the Mima build broken (https://github.com/apache/spark/actions/runs/3380379538/jobs/5613108397) ``` [error] spark-core: Failed binary compatibility check against org.apache.spark:spark-core_2.13:3.3.0! Found 2 potential problems (filtered 945) [error] * method productElementName(Int)java.lang.String in object org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown.productElementName") [error] * method productElementNames()scala.collection.Iterator in object org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown.productElementNames") ``` ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? CI in this PR should test it out. After that, scheduled jobs for Scala 2.13 will test this out Closes #38492 from HyukjinKwon/SPARK-38270-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

… client side ### What changes were proposed in this pull request? Current Spark SQL CLI alway use shutdown hook to stop SparkSQLEnv ``` // Clean up after we exit ShutdownHookManager.addShutdownHook { () => SparkSQLEnv.stop() } ``` but use process ret to close client side jvm ``` while (line != null) { if (!line.startsWith("--")) { if (prefix.nonEmpty) { prefix += '\n' } if (line.trim().endsWith(";") && !line.trim().endsWith("\\;")) { line = prefix + line ret = cli.processLine(line, true) prefix = "" currentPrompt = promptWithCurrentDB } else { prefix = prefix + line currentPrompt = continuedPromptWithDBSpaces } } line = reader.readLine(currentPrompt + "> ") } sessionState.close() System.exit(ret) } ``` ``` if (sessionState.execString != null) { exitCode = cli.processLine(sessionState.execString) System.exit(exitCode) } try { if (sessionState.fileName != null) { exitCode = cli.processFile(sessionState.fileName) System.exit(exitCode) } } catch { case e: FileNotFoundException => logError(s"Could not open input file for reading. (${e.getMessage})") exitCode = 3 System.exit(exitCode) } ``` This always cause client side exit code not consistent with AM. IN this pr I prefer to pass the exit code to `SparkContext.stop()` method to pass a clear client side status to AM side in client mode. In this pr, I add a new `stop` method in `SchedulerBackend` ``` def stop(exitCode: Int): Unit = stop() ``` So we don't need to implement it for all kinds of scheduler backend. In this pr, we only handle the case me meet for `YarnClientSchedulerBackend`, then we can only implement `stop(exitCode: Int)` for this class, then for yarn client mode, it can work now. I think this can benefit many similar case in future. ### Why are the changes needed? Keep client side status consistent with AM side ### Does this PR introduce _any_ user-facing change? With this pr, client side status will be same with am side ### How was this patch tested? MT Closes apache#35594 from AngersZhuuuu/SPARK-38270. Lead-authored-by: Angerszhuuuu <[email protected]> Co-authored-by: AngersZhuuuu <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

…tElementNames in Mima for Scala 2.13 ### What changes were proposed in this pull request? This PR is a followup of apache#35594 that recovers Mima compatibility test for Scala 2.13. ### Why are the changes needed? To fix the Mima build broken (https://github.com/apache/spark/actions/runs/3380379538/jobs/5613108397) ``` [error] spark-core: Failed binary compatibility check against org.apache.spark:spark-core_2.13:3.3.0! Found 2 potential problems (filtered 945) [error] * method productElementName(Int)java.lang.String in object org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown.productElementName") [error] * method productElementNames()scala.collection.Iterator in object org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown does not have a correspondent in current version [error] filter with: ProblemFilters.exclude[DirectMissingMethodProblem]("org.apache.spark.scheduler.cluster.CoarseGrainedClusterMessages#Shutdown.productElementNames") ``` ### Does this PR introduce _any_ user-facing change? No, dev-only. ### How was this patch tested? CI in this PR should test it out. After that, scheduled jobs for Scala 2.13 will test this out Closes apache#38492 from HyukjinKwon/SPARK-38270-followup. Authored-by: Hyukjin Kwon <[email protected]> Signed-off-by: Hyukjin Kwon <[email protected]>

[SPARK-38270][SQL] Spark SQL CLI's AM should keep same exit code with…

dfb5ae4

… client side

github-actions bot added CORE SQL YARN labels Feb 21, 2022

AngersZhuuuu added 5 commits February 21, 2022 17:03

Update SparkSQLCLIDriver.scala

2517c7e

update

23f4f85

update

1b4ee13

update

69bbafa

Update Main.scala

3ac12c0

github-actions bot added the SPARK SHELL label Feb 21, 2022

Update KafkaRDDSuite.scala

4b3ee98

AngersZhuuuu added 10 commits February 22, 2022 11:24

Revert "Update KafkaRDDSuite.scala"

5de5c2a

This reverts commit 4b3ee98.

Revert "Update Main.scala"

a6c4ef4

This reverts commit 3ac12c0.

Revert "update"

2f71ad1

This reverts commit 69bbafa.

Revert "update"

8c12f88

This reverts commit 1b4ee13.

Update SparkContext.scala

d82bf52

Update SparkContext.scala

a28ac42

Revert "update"

bbb3440

This reverts commit 23f4f85.

update

b17833b

update

d06bbf3

Update MimaExcludes.scala

b8c9f4c

github-actions bot added the BUILD label Feb 22, 2022

cloud-fan reviewed Feb 22, 2022

View reviewed changes

core/src/main/scala/org/apache/spark/SparkContext.scala Show resolved Hide resolved

Update SparkContext.scala

0ba9be6

yaooqinn reviewed Feb 22, 2022

View reviewed changes

resource-managers/yarn/src/main/scala/org/apache/spark/deploy/yarn/ApplicationMaster.scala Outdated Show resolved Hide resolved

address comment

73b3c5d

Update SparkContext.scala

ac8f0ed

yaooqinn approved these changes Feb 22, 2022

View reviewed changes

LuciferYang reviewed Feb 22, 2022

View reviewed changes

...e-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala Outdated Show resolved Hide resolved

LuciferYang reviewed Feb 22, 2022

View reviewed changes

...e-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkSQLCLIDriver.scala Outdated Show resolved Hide resolved

LuciferYang approved these changes Feb 22, 2022

View reviewed changes

AngersZhuuuu added 2 commits March 10, 2022 21:02

Merge branch 'master' into SPARK-38270

308c778

Merge branch 'master' into SPARK-38270

c45cc67

github-actions bot added the Stale label Sep 27, 2022

github-actions bot closed this Sep 28, 2022

cloud-fan removed the Stale label Sep 28, 2022

cloud-fan reopened this Sep 28, 2022

Merge branch 'master' into SPARK-38270

355380a

yaooqinn approved these changes Sep 28, 2022

View reviewed changes

Merge branch 'master' into SPARK-38270

6510ef1

cloud-fan closed this in 026b3d4 Nov 1, 2022

HyukjinKwon mentioned this pull request Nov 3, 2022

[SPARK-38270][BUILD][FOLLOW-UP] Exclude productElementName and productElementNames in Mima for Scala 2.13 #38492

Closed

AngersZhuuuu mentioned this pull request Mar 7, 2023

[SPARK-42699][CONNECT] SparkConnectServer should make client and AM same exit code #40315

Closed

[SPARK-38270][SQL] Spark SQL CLI's AM should keep same exit code with client side #35594

[SPARK-38270][SQL] Spark SQL CLI's AM should keep same exit code with client side #35594

Uh oh!

Conversation

AngersZhuuuu commented Feb 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

LuciferYang commented Feb 21, 2022

Uh oh!

AngersZhuuuu commented Feb 22, 2022

Uh oh!

AngersZhuuuu commented Feb 22, 2022

Uh oh!

Uh oh!

cloud-fan commented Feb 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LuciferYang left a comment

Choose a reason for hiding this comment

Uh oh!

AngersZhuuuu commented Feb 28, 2022

Uh oh!

bogdanghit commented Feb 28, 2022

Uh oh!

AngersZhuuuu commented Mar 1, 2022

Uh oh!

AngersZhuuuu commented Mar 29, 2022

Uh oh!

github-actions bot commented Sep 27, 2022

Uh oh!

cloud-fan commented Sep 28, 2022

Uh oh!

yaooqinn commented Sep 28, 2022

Uh oh!

AngersZhuuuu commented Sep 28, 2022

Uh oh!

AngersZhuuuu commented Oct 28, 2022

Uh oh!

cloud-fan commented Nov 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AngersZhuuuu commented Feb 21, 2022 •

edited

Loading

cloud-fan commented Feb 22, 2022 •

edited

Loading