-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-17443][SPARK-11035] Stop Spark Application if launcher goes down and use reflection #15009
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 27 commits
8986aa9
99b1d1b
70a67fb
bac5cd2
38a7e3a
67f8de7
57defa9
eaa1bca
a3250ac
58c6bac
0a57684
1fe498b
25c3258
14050f5
92e4445
64a21b3
1e6311a
c207b30
ad20ccc
6a7ba5b
3cb8e85
3091105
2fdcec9
64a0e45
4357107
2707d21
cc2c0be
82df055
99d8c29
41cf6da
b098ecd
b66243d
bc99435
517fed0
a3d18b4
c17f15f
026d026
fe5b5d6
677edf7
ee3f24a
30b460c
7323200
0cfd4a7
8609874
ab50dd8
14c6365
04e56fc
7ee465f
3f060b6
f1b49d8
2996fb1
a311721
d906072
81fd297
10513ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark | ||
|
|
||
| /** | ||
| * A client that SparkSubmit uses to launch spark Application. | ||
| * This is currently supported only in YARN mode. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No need to add this. Listing the existing implementations in an interface's javadoc is not really interesting. |
||
| */ | ||
| private[spark] trait SparkApp { | ||
| this: Singleton => | ||
|
|
||
| /** | ||
| * The Client should implement this as entry method to provide application, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead: "Method executed by SparkSubmit to run the application." |
||
| * spark conf and system configuration. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Comment is now stale (there's no "system configuration" parameter). |
||
| * | ||
| * @param args - all arguments for SparkApp. | ||
| * @param conf - Spark Configuration. | ||
| * @param envvars - system environment Variables. | ||
| */ | ||
| def sparkMain( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add scaladoc to this method? It's important to explain what each argument is here. |
||
| args: Array[String], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: the method declaration now fits in a single line |
||
| conf: scala.collection.immutable.Map[String, String], | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't need the fully qualified name here. |
||
| envvars: scala.collection.immutable.Map[String, String]): Unit | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,7 @@ import java.security.PrivilegedExceptionAction | |
| import java.text.ParseException | ||
|
|
||
| import scala.annotation.tailrec | ||
| import scala.collection.JavaConverters._ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this being used anywhere? |
||
| import scala.collection.mutable.{ArrayBuffer, HashMap, Map} | ||
| import scala.util.Properties | ||
|
|
||
|
|
@@ -685,9 +686,8 @@ object SparkSubmit extends CommandLineUtils { | |
| addJarToClasspath(jar, loader) | ||
| } | ||
|
|
||
| for ((key, value) <- sysProps) { | ||
| System.setProperty(key, value) | ||
| } | ||
| val threadEnabled = sysProps.getOrElse(SparkLauncher.LAUNCHER_INTERNAL_THREAD_ENABLED, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would move this down to closer to where its used. |
||
| "false").toBoolean | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove extra newline |
||
| var mainClass: Class[_] = null | ||
|
|
||
|
|
@@ -719,7 +719,23 @@ object SparkSubmit extends CommandLineUtils { | |
| printWarning("Subclasses of scala.App may not work correctly. Use a main() method instead.") | ||
| } | ||
|
|
||
| val mainMethod = mainClass.getMethod("main", new Array[String](0).getClass) | ||
| val sparkAppMainMethodArr = mainClass.getMethods().filter{_.getName() == "sparkMain"} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You haven't addressed this. |
||
| val isSparkApp = sparkAppMainMethodArr.length > 0 | ||
|
|
||
| val childSparkConf = sysProps.filter{ p => p._1.startsWith("spark.")}.toMap | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space after |
||
| // If running sparkApp or in thread mode, the System properties should not be cluttered. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can remove "or in thread mode"
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also perhaps reword this a bit If running a SparkApp we can explicitly pass in the confs separately. If we aren't running a SparkApp they get passed via the system properties. |
||
| // This helps keep clean isolation between multiple Spark Apps launched in different threads. | ||
| if (!isSparkApp || !threadEnabled) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should add a comment here explaining this
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should be
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. actually shouldn't this just be !isSparkApp? because if it is the spark app we are going to send sparkconf separate from the sys.env so we don't need to send them both in childSparkConf and in the system properties. If that is the case we don't need threadEnabled at all here. |
||
| sysProps.foreach { case (key, value) => | ||
| System.setProperty(key, value) | ||
| } | ||
| } | ||
|
|
||
| val mainMethod = if (isSparkApp) { | ||
| sparkAppMainMethodArr(0) | ||
| } else { | ||
| mainClass.getMethod("main", new Array[String](0).getClass) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not your fault, but |
||
| } | ||
| if (!Modifier.isStatic(mainMethod.getModifiers)) { | ||
| throw new IllegalStateException("The main method in the given main class must be static") | ||
| } | ||
|
|
@@ -735,7 +751,11 @@ object SparkSubmit extends CommandLineUtils { | |
| } | ||
|
|
||
| try { | ||
| mainMethod.invoke(null, childArgs.toArray) | ||
| if (isSparkApp) { | ||
| mainMethod.invoke(null, childArgs.toArray, childSparkConf, sys.env) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you're passing |
||
| } else { | ||
| mainMethod.invoke(null, childArgs.toArray) | ||
| } | ||
| } catch { | ||
| case t: Throwable => | ||
| findCause(t) match { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,38 +17,67 @@ | |
|
|
||
| package org.apache.spark.launcher | ||
|
|
||
| import java.io.IOException | ||
| import java.net.{InetAddress, Socket} | ||
|
|
||
| import org.apache.spark.SPARK_VERSION | ||
| import org.apache.spark.internal.Logging | ||
| import org.apache.spark.launcher.LauncherProtocol._ | ||
| import org.apache.spark.util.{ThreadUtils, Utils} | ||
| import org.apache.spark.util.{ShutdownHookManager, ThreadUtils, Utils} | ||
|
|
||
| /** | ||
| * A class that can be used to talk to a launcher server. Users should extend this class to | ||
| * provide implementation for the abstract methods. | ||
| * | ||
| * See `LauncherServer` for an explanation of how launcher communication works. | ||
| */ | ||
| private[spark] abstract class LauncherBackend { | ||
| private[spark] abstract class LauncherBackend extends Logging { | ||
|
|
||
| private var clientThread: Thread = _ | ||
| private var connection: BackendConnection = _ | ||
| private var lastState: SparkAppHandle.State = _ | ||
| private var stopOnShutdown: Boolean = false | ||
| @volatile private var _isConnected = false | ||
|
|
||
| def connect(): Unit = { | ||
| val port = sys.env.get(LauncherProtocol.ENV_LAUNCHER_PORT).map(_.toInt) | ||
| val secret = sys.env.get(LauncherProtocol.ENV_LAUNCHER_SECRET) | ||
| val stopFlag = sys.env.get(LauncherProtocol.ENV_LAUNCHER_STOP_IF_SHUTDOWN).map(_.toBoolean) | ||
| if (port != None && secret != None) { | ||
| val s = new Socket(InetAddress.getLoopbackAddress(), port.get) | ||
| connection = new BackendConnection(s) | ||
| connection.send(new Hello(secret.get, SPARK_VERSION)) | ||
| clientThread = LauncherBackend.threadFactory.newThread(connection) | ||
| clientThread.start() | ||
| _isConnected = true | ||
| connect(port.get, secret.get, stopFlag.getOrElse(false)) | ||
| } | ||
| } | ||
|
|
||
| def connect(port: Int, secret: String): Unit = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method (but not the code!) seems redundant. If you had just |
||
| val s = new Socket(InetAddress.getLoopbackAddress(), port) | ||
| connection = new BackendConnection(s) | ||
| connection.send(new Hello(secret, SPARK_VERSION)) | ||
| clientThread = LauncherBackend.threadFactory.newThread(connection) | ||
| clientThread.start() | ||
| _isConnected = true | ||
| if (stopOnShutdown) { | ||
| logDebug("Adding shutdown hook") // force eager creation of logger | ||
| var _shutdownHookRef = ShutdownHookManager.addShutdownHook( | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're not using |
||
| ShutdownHookManager.SPARK_CONTEXT_SHUTDOWN_PRIORITY) { () => | ||
| logInfo("Invoking onStopRequest() from shutdown hook") | ||
| try { | ||
| if (_isConnected) { | ||
| onStopRequest() | ||
| } | ||
| } | ||
| catch { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: move to previous line |
||
| case anotherIOE: IOException => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why |
||
| logError("Error while running LauncherBackend shutdownHook...", anotherIOE) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| def connect(port: Int, secret: String, stopFlag: Boolean): Unit = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You haven't addressed my previous feedback here: #15009 (comment)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This addressed now.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You haven't addressed it, one way or another. You don't need all these |
||
| this.stopOnShutdown = stopFlag | ||
| connect(port, secret) | ||
| } | ||
|
|
||
| def close(): Unit = { | ||
| if (connection != null) { | ||
| try { | ||
|
|
@@ -71,6 +100,9 @@ private[spark] abstract class LauncherBackend { | |
| if (connection != null && lastState != state) { | ||
| connection.send(new SetState(state)) | ||
| lastState = state | ||
| if (!_isConnected && stopOnShutdown) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You haven't addressed my previous comment here: #15009 (comment) If the backend is not connected to the launcher, it may mean the launcher has explicitly disconnected from it. So the launcher explicitly wants the app to keep running when the connection is closed. This seems to be breaking that. Can you clarify what you're trying to achieve here? It feels to me like you want the launcher to stop the application when the launcher's JVM exits. And that means handling this in the launcher code, not here.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens with code like this:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need this check at all here vs just relying on the close() call? _isConnected only becomes false after calling that which should do the fireStopRequest(), am I missing a case? If it wasn't connected at all on startup the connection != null case wouldn't pass
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. on the other issue of disconnect() I would think that if we just document that behavior its fine. I hadn't noticed before that the disconnect java docs state "Disconnects the handle from the application, without stopping it." |
||
| fireStopRequest() | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -110,12 +142,14 @@ private[spark] abstract class LauncherBackend { | |
| override def close(): Unit = { | ||
| try { | ||
| super.close() | ||
| if (stopOnShutdown) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See previous comment for why I don't think this is correct. |
||
| fireStopRequest() | ||
| } | ||
| } finally { | ||
| onDisconnected() | ||
| _isConnected = false | ||
| } | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.launcher; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.ArrayList; | ||
| import java.util.List; | ||
| import java.util.logging.Level; | ||
| import java.util.logging.Logger; | ||
|
|
||
| public abstract class AbstractSparkAppHandle implements SparkAppHandle { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These classes should not be public; only |
||
| private static final Logger LOG = Logger.getLogger(AbstractSparkAppHandle.class.getName()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nevermind, forgot this is java.util.logging not slf4j... |
||
|
|
||
| protected final String secret; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: please add a blank line between static and non-static fields. |
||
| protected final LauncherServer server; | ||
| protected boolean disposed; | ||
| protected List<Listener> listeners; | ||
| protected State state; | ||
| private LauncherConnection connection; | ||
| private String appId; | ||
| OutputRedirector redirector; | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove extra blank newlines |
||
| public AbstractSparkAppHandle(LauncherServer server, String secret) { | ||
| this.server = server; | ||
| this.secret = secret; | ||
| this.state = State.UNKNOWN; | ||
| } | ||
|
|
||
| @Override | ||
| public synchronized void addListener(Listener l) { | ||
| if (listeners == null) { | ||
| listeners = new ArrayList<>(); | ||
| } | ||
| listeners.add(l); | ||
| } | ||
|
|
||
| @Override | ||
| public State getState() { | ||
| return state; | ||
| } | ||
|
|
||
| @Override | ||
| public String getAppId() { | ||
| return appId; | ||
| } | ||
|
|
||
| @Override | ||
| public void stop() { | ||
| CommandBuilderUtils.checkState(connection != null, "Application is still not connected."); | ||
| try { | ||
| connection.send(new LauncherProtocol.Stop()); | ||
| } catch (IOException ioe) { | ||
| throw new RuntimeException(ioe); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public synchronized void disconnect() { | ||
| if (!disposed) { | ||
| disposed = true; | ||
| if (connection != null) { | ||
| try { | ||
| connection.close(); | ||
| } catch (IOException ioe) { | ||
| // no-op. | ||
| } | ||
| } | ||
| server.unregister(this); | ||
| if (redirector != null) { | ||
| redirector.stop(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| String getSecret() { | ||
| return secret; | ||
| } | ||
|
|
||
| void setConnection(LauncherConnection connection) { | ||
| this.connection = connection; | ||
| } | ||
|
|
||
| LauncherServer getServer() { | ||
| return server; | ||
| } | ||
|
|
||
| LauncherConnection getConnection() { | ||
| return connection; | ||
| } | ||
|
|
||
| void setState(State s) { | ||
| if (!state.isFinal()) { | ||
| state = s; | ||
| fireEvent(false); | ||
| } else { | ||
| LOG.log(Level.WARNING, "Backend requested transition from final state {0} to {1}.", | ||
| new Object[]{state, s}); | ||
| } | ||
| } | ||
|
|
||
| void setAppId(String appId) { | ||
| this.appId = appId; | ||
| fireEvent(true); | ||
| } | ||
|
|
||
| private synchronized void fireEvent(boolean isInfoChanged) { | ||
| if (listeners != null) { | ||
| for (Listener l : listeners) { | ||
| if (isInfoChanged) { | ||
| l.infoChanged(this); | ||
| } else { | ||
| l.stateChanged(this); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment seems very specific to the YARN "Client" class.
Instead: "An interface that can be implemented by applications launched by SparkSubmit which exposes the Spark job configuration explicitly."