You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by vanzin <gi...@git.apache.org> on 2017/10/27 22:47:09 UTC

[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

GitHub user vanzin opened a pull request:

    https://github.com/apache/spark/pull/19591

    [SPARK-11035][core] Add in-process Spark app launcher.

    This change adds a new launcher that allows applications to be run
    in a separate thread in the same process as the calling code. To
    achieve that, some code from the child process implementation was
    moved to abstract classes that implement the common functionality,
    and the new launcher inherits from those.
    
    The new launcher was added as a new class, instead of implemented
    as a new option to the existing SparkLauncher, to avoid ambigous
    APIs. For example, SparkLauncher has ways to set the child app's
    environment, modify SPARK_HOME, or control the logging of the
    child process, none of which apply to in-process apps.
    
    The in-process launcher has limitations: it needs Spark in the
    context class loader of the calling thread, and it's bound by
    Spark's current limitation of a single client-mode application
    per JVM. It also relies on the recently added SparkApplication
    trait to make sure different apps don't mess up each other's
    configuration, but currently no cluster manager client implements
    that.
    
    I also chose to keep the same socket-based communication for in-process
    apps, even though it might be possible to avoid it for in-process
    mode. That helps both implementations share more code.
    
    Tested with new and existing unit tests, and with a simple app that
    uses the launcher; also made sure the app ran fine with older launcher
    jar to check binary compatibility.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/vanzin/spark SPARK-11035

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/spark/pull/19591.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #19591
    
----
commit 928a0520677ea89aaacb59d48dd08b52b80462ad
Author: Marcelo Vanzin <va...@cloudera.com>
Date:   2017-08-15T21:39:23Z

    [SPARK-11035][core] Add in-process Spark app launcher.
    
    This change adds a new launcher that allows applications to be run
    in a separate thread in the same process as the calling code. To
    achieve that, some code from the child process implementation was
    moved to abstract classes that implement the common functionality,
    and the new launcher inherits from those.
    
    The new launcher was added as a new class, instead of implemented
    as a new option to the existing SparkLauncher, to avoid ambigous
    APIs. For example, SparkLauncher has ways to set the child app's
    environment, modify SPARK_HOME, or control the logging of the
    child process, none of which apply to in-process apps.
    
    The in-process launcher has limitations: it needs Spark in the
    context class loader of the calling thread, and it's bound by
    Spark's current limitation of a single client-mode application
    per JVM. It also relies on the recently added SparkApplication
    trait to make sure different apps don't mess up each other's
    configuration, but currently no cluster manager client implements
    that.
    
    I also chose to keep the same socket-based communication for in-process
    apps, even though it might be possible to avoid it for in-process
    mode. That helps both implementations share more code.
    
    Tested with new and existing unit tests, and with a simple app that
    uses the launcher; also made sure the app ran fine with older launcher
    jar to check binary compatibility.

----


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84666/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by justinuang <gi...@git.apache.org>.
Github user justinuang commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Really looking forward to this PR! For our use case, it will reduce our spark launch times by ~4 seconds.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158537984
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.util.concurrent.ThreadFactory;
    +import java.util.logging.Level;
    +import java.util.logging.Logger;
    +
    +class InProcessAppHandle extends AbstractAppHandle {
    +
    +  private static final Logger LOG = Logger.getLogger(ChildProcAppHandle.class.getName());
    +  private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("spark-app-%d");
    +
    +  private Thread app;
    +
    +  InProcessAppHandle(LauncherServer server) {
    +    super(server);
    +  }
    +
    +  @Override
    +  public synchronized void kill() {
    +    LOG.warning("kill() may leave the underlying app running in in-process mode.");
    +    disconnect();
    +
    +    // Interrupt the thread. This is not guaranteed to kill the app, though.
    --- End diff --
    
    `SparkSubmit` just runs some other class, in this case, the class that submits the app in cluster mode (or the user class in client mode). And that class could swallow these interrupts, just as `SparkSubmit` also could (but doesn't?).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84442 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84442/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84657/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158200163
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.util.concurrent.ThreadFactory;
    +import java.util.logging.Level;
    +import java.util.logging.Logger;
    +
    +class InProcessAppHandle extends AbstractAppHandle {
    +
    +  private static final Logger LOG = Logger.getLogger(ChildProcAppHandle.class.getName());
    +  private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("spark-app-%d");
    +
    +  private Thread app;
    +
    +  InProcessAppHandle(LauncherServer server) {
    +    super(server);
    +  }
    +
    +  @Override
    +  public synchronized void kill() {
    +    LOG.warning("kill() may leave the underlying app running in in-process mode.");
    +    disconnect();
    +
    +    // Interrupt the thread. This is not guaranteed to kill the app, though.
    --- End diff --
    
    in cluster mode, shouldn't this be pretty safe?  If not, wouldn't it be a bug in spark-submit?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158509926
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.util.concurrent.ThreadFactory;
    +import java.util.logging.Level;
    +import java.util.logging.Logger;
    +
    +class InProcessAppHandle extends AbstractAppHandle {
    +
    +  private static final Logger LOG = Logger.getLogger(ChildProcAppHandle.class.getName());
    +  private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("spark-app-%d");
    +
    +  private Thread app;
    +
    +  InProcessAppHandle(LauncherServer server) {
    +    super(server);
    +  }
    +
    +  @Override
    +  public synchronized void kill() {
    +    LOG.warning("kill() may leave the underlying app running in in-process mode.");
    +    disconnect();
    +
    +    // Interrupt the thread. This is not guaranteed to kill the app, though.
    --- End diff --
    
    sorry I don't understand.  I don't see why the client that does the submission would matter.  I thought you'd have problems if the interrupt was caught and swallowed by spark-submit.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84442 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84442/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84721 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84721/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).
     * This patch **fails SparkR unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84657 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84657/testReport)** for PR 19591 at commit [`8013766`](https://github.com/apache/spark/commit/8013766d730b9fa14b9d0c71d527dfcfcead8af1).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84498 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84498/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84721/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84877 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84877/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r156746267
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessLauncher.java ---
    @@ -0,0 +1,108 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.lang.reflect.Modifier;
    +import java.util.List;
    +import java.util.logging.Logger;
    +
    +/**
    + * In-process launcher for Spark applications.
    + * <p>
    + * Use this class to start Spark applications programmatically. Applications launched using this
    + * class will run in the same process as the caller.
    + * <p>
    + * Because Spark only supports a single active instance of <code>SparkContext</code> per JVM, code
    + * that uses this class should be careful about which applications are launched. It's recommended
    + * that this launcher only be used to launch applications in cluster mode.
    + * <p>
    + * Also note that, when running applications in client mode, JVM-related configurations (like
    + * driver memory or configs which modify the driver's class path) do not take effect. Logging
    + * configuration is also inherited from the parent application.
    + *
    + * @since Spark 2.3.0
    + */
    +public class InProcessLauncher extends AbstractLauncher<InProcessLauncher> {
    +
    +  private static final Logger LOG = Logger.getLogger(InProcessLauncher.class.getName());
    +
    +  /**
    +   * Starts a Spark application.
    +   *
    +   * @see AbstractLauncher#startApplication(SparkAppHandle.Listener...)
    +   * @param listeners Listeners to add to the handle before the app is launched.
    +   * @return A handle for the launched application.
    +   */
    +  @Override
    +  public SparkAppHandle startApplication(SparkAppHandle.Listener... listeners) throws IOException {
    +    if (builder.isClientMode(builder.getEffectiveConfig())) {
    +      LOG.warning("It's not recommended to run client-mode applications using InProcessLauncher.");
    +    }
    +
    --- End diff --
    
    You'll already get a ton of logs from SparkSubmit (or an exception if it doesn't run).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #83655 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83655/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84783 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84783/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158316971
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
    @@ -210,8 +209,13 @@ int getPort() {
        * Removes the client handle from the pending list (in case it's still there), and unrefs
        * the server.
        */
    -  void unregister(ChildProcAppHandle handle) {
    -    pending.remove(handle.getSecret());
    +  void unregister(AbstractAppHandle handle) {
    +    for (Map.Entry<String, AbstractAppHandle> e : pending.entrySet()) {
    --- End diff --
    
    ok one more try --
    
    you are generating a secret whether its in-process or a child process, so why not store that secret in AbstractAppHandle?  this isn't really a big deal, the efficiency difference doesn't matter, but the one line `pending.remove(handle.getSecret())` is also easier to follow.
    
    also could you rename `pending` to `secretToPendingApps` so its a more clear what the key is.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158349922
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.util.concurrent.ThreadFactory;
    +import java.util.logging.Level;
    +import java.util.logging.Logger;
    +
    +class InProcessAppHandle extends AbstractAppHandle {
    +
    +  private static final Logger LOG = Logger.getLogger(ChildProcAppHandle.class.getName());
    +  private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("spark-app-%d");
    +
    +  private Thread app;
    +
    +  InProcessAppHandle(LauncherServer server) {
    +    super(server);
    +  }
    +
    +  @Override
    +  public synchronized void kill() {
    +    LOG.warning("kill() may leave the underlying app running in in-process mode.");
    +    disconnect();
    +
    +    // Interrupt the thread. This is not guaranteed to kill the app, though.
    --- End diff --
    
    It depends on the implementation of the client that does the submission, not spark-submit, but it should be safe and you could consider it a bug if it doesn't work.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #83137 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83137/testReport)** for PR 19591 at commit [`928a052`](https://github.com/apache/spark/commit/928a0520677ea89aaacb59d48dd08b52b80462ad).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/spark/pull/19591


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    retest this please



---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    ack will try to get to this tomorrow


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84442/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    @tgravescs (who reviewed the original change for this bug), @srowen @jerryshao 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Looks like a legitimate flaky test. Will take a look.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84792 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84792/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).
     * This patch **fails Scala style tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84629 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84629/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158560723
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.util.concurrent.ThreadFactory;
    +import java.util.logging.Level;
    +import java.util.logging.Logger;
    +
    +class InProcessAppHandle extends AbstractAppHandle {
    +
    +  private static final Logger LOG = Logger.getLogger(ChildProcAppHandle.class.getName());
    +  private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("spark-app-%d");
    +
    +  private Thread app;
    +
    +  InProcessAppHandle(LauncherServer server) {
    +    super(server);
    +  }
    +
    +  @Override
    +  public synchronized void kill() {
    +    LOG.warning("kill() may leave the underlying app running in in-process mode.");
    +    disconnect();
    +
    +    // Interrupt the thread. This is not guaranteed to kill the app, though.
    --- End diff --
    
    ok, just a misunderstanding then.  that is what I thought I was saying in the first place.
    
    anyway, I guess we can leave the warning here for now in all cases and see if we get any reports from users in cluster mode ...


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84677/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158200059
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/ChildProcAppHandle.java ---
    @@ -171,10 +90,11 @@ void monitorChild() {
         }
     
         synchronized (this) {
    -      if (disposed) {
    +      if (isDisposed()) {
             return;
           }
     
    +      State currState = getState();
    --- End diff --
    
    this was added just because `currState` is no longer accessible, right?  You're not particularly trying to grab the state before the call to `disconnect()`?  Might be clearly to move it after, otherwise on first read it looks like it is trying to grab the state before its modified by `disconnect()`


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84666 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84666/testReport)** for PR 19591 at commit [`8013766`](https://github.com/apache/spark/commit/8013766d730b9fa14b9d0c71d527dfcfcead8af1).
     * This patch **fails Spark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158351004
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
    @@ -260,24 +264,30 @@ private long getConnectionTimeout() {
       }
     
       private String createSecret() {
    -    byte[] secret = new byte[128];
    -    RND.nextBytes(secret);
    -
    -    StringBuilder sb = new StringBuilder();
    -    for (byte b : secret) {
    -      int ival = b >= 0 ? b : Byte.MAX_VALUE - b;
    -      if (ival < 0x10) {
    -        sb.append("0");
    +    while (true) {
    --- End diff --
    
    Not really, it's mostly moving the logic that existed before (look for `while (server.pending.containsKey(secret))`).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84498/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84792/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84721 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84721/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by rezasafi <gi...@git.apache.org>.
Github user rezasafi commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r156535010
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessLauncher.java ---
    @@ -0,0 +1,108 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.lang.reflect.Modifier;
    +import java.util.List;
    +import java.util.logging.Logger;
    +
    +/**
    + * In-process launcher for Spark applications.
    + * <p>
    + * Use this class to start Spark applications programmatically. Applications launched using this
    + * class will run in the same process as the caller.
    + * <p>
    + * Because Spark only supports a single active instance of <code>SparkContext</code> per JVM, code
    + * that uses this class should be careful about which applications are launched. It's recommended
    + * that this launcher only be used to launch applications in cluster mode.
    + * <p>
    + * Also note that, when running applications in client mode, JVM-related configurations (like
    + * driver memory or configs which modify the driver's class path) do not take effect. Logging
    + * configuration is also inherited from the parent application.
    + *
    + * @since Spark 2.3.0
    + */
    +public class InProcessLauncher extends AbstractLauncher<InProcessLauncher> {
    +
    +  private static final Logger LOG = Logger.getLogger(InProcessLauncher.class.getName());
    +
    +  /**
    +   * Starts a Spark application.
    +   *
    +   * @see AbstractLauncher#startApplication(SparkAppHandle.Listener...)
    +   * @param listeners Listeners to add to the handle before the app is launched.
    +   * @return A handle for the launched application.
    +   */
    +  @Override
    +  public SparkAppHandle startApplication(SparkAppHandle.Listener... listeners) throws IOException {
    +    if (builder.isClientMode(builder.getEffectiveConfig())) {
    +      LOG.warning("It's not recommended to run client-mode applications using InProcessLauncher.");
    +    }
    +
    --- End diff --
    
    Just maybe a LOG.debug that shows an in-process app is started will be useful.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84877 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84877/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    I'll leave this up a little longer to see if anyone volunteers to review, otherwise I'll ping some random people.   


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84792 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84792/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Ping.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84629 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84629/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    @vanzin, sorry I've been swamped and haven't had a chance to get to this. Still on my list but can't guarantee time frame.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #83137 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83137/testReport)** for PR 19591 at commit [`928a052`](https://github.com/apache/spark/commit/928a0520677ea89aaacb59d48dd08b52b80462ad).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds the following public classes _(experimental)_:
      * `abstract class AbstractAppHandle implements SparkAppHandle `
      * `public abstract class AbstractLauncher<T extends AbstractLauncher> `
      * `   * It is safe to add arguments modified by other methods in this class (such as`
      * `class ChildProcAppHandle extends AbstractAppHandle `
      * `class InProcessAppHandle extends AbstractAppHandle `
      * `public class InProcessLauncher extends AbstractLauncher<InProcessLauncher> `
      * `public class SparkLauncher extends AbstractLauncher<SparkLauncher> `


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158307838
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
    @@ -210,8 +209,13 @@ int getPort() {
        * Removes the client handle from the pending list (in case it's still there), and unrefs
        * the server.
        */
    -  void unregister(ChildProcAppHandle handle) {
    -    pending.remove(handle.getSecret());
    +  void unregister(AbstractAppHandle handle) {
    +    for (Map.Entry<String, AbstractAppHandle> e : pending.entrySet()) {
    --- End diff --
    
    nevermind, stupid idea


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84498 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84498/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    A note about the implementation: since this is executing `SparkSubmit` under the covers, it's possible to call the new `InProcessLauncher` and cause it to exit the current JVM because there's an error with the user-provided args. That's not optimal, but to avoid growing the current PR too much I'll leave the proper fix for that to a separate change.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #85279 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85279/testReport)** for PR 19591 at commit [`5dd5b5d`](https://github.com/apache/spark/commit/5dd5b5db112ac99f3e70ce50db5e5196379c3610).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #83655 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/83655/testReport)** for PR 19591 at commit [`8496024`](https://github.com/apache/spark/commit/84960244458045810f0066256fbbee2446a3071c).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84629/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158201917
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/package-info.java ---
    @@ -49,6 +49,15 @@
      * </pre>
      *
      * <p>
    + * Applications can also be launched in-process by using
    + * {@link org.apache.spark.launcher.InProcessLauncher} instead. Launching applications in-process
    --- End diff --
    
    comment above about "there is only one entry point" is a bit incorrect now


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83655/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84657 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84657/testReport)** for PR 19591 at commit [`8013766`](https://github.com/apache/spark/commit/8013766d730b9fa14b9d0c71d527dfcfcead8af1).
     * This patch **fails PySpark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158355602
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
    @@ -210,8 +209,13 @@ int getPort() {
        * Removes the client handle from the pending list (in case it's still there), and unrefs
        * the server.
        */
    -  void unregister(ChildProcAppHandle handle) {
    -    pending.remove(handle.getSecret());
    +  void unregister(AbstractAppHandle handle) {
    +    for (Map.Entry<String, AbstractAppHandle> e : pending.entrySet()) {
    --- End diff --
    
    I think I had something like that at some point, but it was more code than the current version... there's a little bit of a chicken & egg problem between handles and secrets, and keeping them separate simplified things a bit at least for me.
    
    I'll do the rename. 


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #85279 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/85279/testReport)** for PR 19591 at commit [`5dd5b5d`](https://github.com/apache/spark/commit/5dd5b5db112ac99f3e70ce50db5e5196379c3610).
     * This patch passes all tests.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158201527
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
    @@ -260,24 +264,30 @@ private long getConnectionTimeout() {
       }
     
       private String createSecret() {
    -    byte[] secret = new byte[128];
    -    RND.nextBytes(secret);
    -
    -    StringBuilder sb = new StringBuilder();
    -    for (byte b : secret) {
    -      int ival = b >= 0 ? b : Byte.MAX_VALUE - b;
    -      if (ival < 0x10) {
    -        sb.append("0");
    +    while (true) {
    --- End diff --
    
    checking my understanding -- extra bug fix here?  even in old code, if by chance two apps had same secret, you'd end up losing one handle?


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test FAILed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84783/
    Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84783 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84783/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Failure looks unrelated... retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    retest this please


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/83137/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84666 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84666/testReport)** for PR 19591 at commit [`8013766`](https://github.com/apache/spark/commit/8013766d730b9fa14b9d0c71d527dfcfcead8af1).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158201743
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/LauncherServer.java ---
    @@ -210,8 +209,13 @@ int getPort() {
        * Removes the client handle from the pending list (in case it's still there), and unrefs
        * the server.
        */
    -  void unregister(ChildProcAppHandle handle) {
    -    pending.remove(handle.getSecret());
    +  void unregister(AbstractAppHandle handle) {
    +    for (Map.Entry<String, AbstractAppHandle> e : pending.entrySet()) {
    --- End diff --
    
    you could add a system.identityHashCode "secret" to the InProcessAppHandle to keep the old version, though this is fine too


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84677 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84677/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).
     * This patch **fails PySpark unit tests**.
     * This patch merges cleanly.
     * This patch adds no public classes.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/85279/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    lgtm


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    **[Test build #84677 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84677/testReport)** for PR 19591 at commit [`ee4098b`](https://github.com/apache/spark/commit/ee4098bf108c8e919b41e392c7316271173e6dc2).


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    merged to master


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158200240
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.util.concurrent.ThreadFactory;
    +import java.util.logging.Level;
    +import java.util.logging.Logger;
    +
    +class InProcessAppHandle extends AbstractAppHandle {
    +
    +  private static final Logger LOG = Logger.getLogger(ChildProcAppHandle.class.getName());
    +  private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("spark-app-%d");
    +
    +  private Thread app;
    +
    +  InProcessAppHandle(LauncherServer server) {
    +    super(server);
    +  }
    +
    +  @Override
    +  public synchronized void kill() {
    +    LOG.warning("kill() may leave the underlying app running in in-process mode.");
    +    disconnect();
    +
    +    // Interrupt the thread. This is not guaranteed to kill the app, though.
    +    if (app != null) {
    +      app.interrupt();
    +    }
    +
    +    setState(State.KILLED);
    +  }
    +
    +  synchronized void start(Method main, String[] args) {
    +    CommandBuilderUtils.checkState(app == null, "Handle already started.");
    +    app = THREAD_FACTORY.newThread(() -> {
    +      try {
    +        main.invoke(null, (Object) args);
    +      } catch (Throwable t) {
    +        LOG.log(Level.WARNING, "Application failed with exception.", t);
    +        setState(State.FAILED);
    +      }
    +
    +      synchronized (InProcessAppHandle.this) {
    +        if (!isDisposed()) {
    +          State currState = getState();
    +          disconnect();
    --- End diff --
    
    same comment here on ordering of getState() & disconnect()


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    > you're talking about removing the System.exit() in SparkSubmit?
    
    Yes, and also potentially changing error messages currently printed to the terminal into exceptions when running through the launcher.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Test PASSed.
    Refer to this link for build results (access rights to CI server needed): 
    https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84877/
    Test PASSed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark issue #19591: [SPARK-11035][core] Add in-process Spark app launcher.

Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:

    https://github.com/apache/spark/pull/19591
  
    Merged build finished. Test FAILed.


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] spark pull request #19591: [SPARK-11035][core] Add in-process Spark app laun...

Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:

    https://github.com/apache/spark/pull/19591#discussion_r158198274
  
    --- Diff: launcher/src/main/java/org/apache/spark/launcher/InProcessAppHandle.java ---
    @@ -0,0 +1,74 @@
    +/*
    + * 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.lang.reflect.Method;
    +import java.util.concurrent.ThreadFactory;
    +import java.util.logging.Level;
    +import java.util.logging.Logger;
    +
    +class InProcessAppHandle extends AbstractAppHandle {
    +
    +  private static final Logger LOG = Logger.getLogger(ChildProcAppHandle.class.getName());
    +  private static final ThreadFactory THREAD_FACTORY = new NamedThreadFactory("spark-app-%d");
    --- End diff --
    
    how about the thread name including `builder.appName`?  might be useful if you are trying to monitor a bunch of threads?
    
    (though you'd also be launching in cluster mode in that case, so the thread wouldn't be doing much ...)


---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org