You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by NiharS <gi...@git.apache.org> on 2018/08/22 23:37:46 UTC
[GitHub] spark pull request #22192: [SPARK-24918] Executor Plugin API
GitHub user NiharS opened a pull request:
https://github.com/apache/spark/pull/22192
[SPARK-24918] Executor Plugin API
A continuation of @squito's executor plugin task. By his request I took his code and added testing and moved the plugin initialization to a separate thread.
## What changes were proposed in this pull request?
Executor plugins now run on one separate thread, so the executor does not wait on them. Added testing.
## How was this patch tested?
Added test cases that test using a sample plugin.
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/NiharS/spark executorPlugin
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22192.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 #22192
----
commit 44454dd586e35bdf16492c4a8969494bd3b7f8f5
Author: Nihar Sheth <ni...@...>
Date: 2018-08-20T17:53:37Z
[SPARK-24918] Executor Plugin API
This commit adds testing and moves the plugin initialization to
a separate thread.
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215371723
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,50 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * hopefully be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all Spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
+
+ /**
+ * Initialization method that will be called during executor startup, in the same thread as
+ * the executor. Plugins should override this method to add in their initialization logic.
+ */
+ default void init() {}
--- End diff --
I think it should be either `init` & `shutdown`, or `start` & `stop`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
> It seems like the line Thread.currentThread().setContextClassLoader(replClassLoader) is causing the pyspark failures
What if you restore the original class loader after initializing the plugins? I was a little worried about this call, but was waiting for tests... so if it's causing problems, better to not change the way things work there.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918] Executor Plugin API
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/22192
Jenkins, ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
Tried on a different machine and those tests pass, here's hoping
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95806 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95806/testReport)** for PR 22192 at commit [`c725a53`](https://github.com/apache/spark/commit/c725a536c45bc73f40cd92c782d3f4975c298517).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95806 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95806/testReport)** for PR 22192 at commit [`c725a53`](https://github.com/apache/spark/commit/c725a536c45bc73f40cd92c782d3f4975c298517).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95565 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95565/testReport)** for PR 22192 at commit [`fa19ea8`](https://github.com/apache/spark/commit/fa19ea880f4fc7eafb736e372479d4df2bffb74b).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215373277
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,50 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * hopefully be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all Spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
+
+ /**
+ * Initialization method that will be called during executor startup, in the same thread as
+ * the executor. Plugins should override this method to add in their initialization logic.
+ */
+ default void init() {}
+
+ /**
+ * Stop method, to be called when the executor is shutting down. Plugins should clean up
--- End diff --
Prefer:
```
Callback for cleaning up executor plugin resources. Called when the executor is shutting down.
```
"Prepare to terminate" is also a little vague; what does that mean? Shouldn't the plugin be terminated already when this method returns? Otherwise there are no guarantees of what happens, right?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
Logs suddenly cut off again without any exceptions, don't think this one is a code error as well. Could someone 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212747628
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ JavaSparkContext sc = null;
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ Assert.assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ } finally {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ JavaSparkContext sc = null;
+ numSuccessfulPlugins = 0;
--- End diff --
I'd do this in a `@Before` method.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95376/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214486075
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,15 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val pluginList = conf.get(EXECUTOR_PLUGINS)
+ if (pluginList != Nil) logDebug(s"Loading the following plugins: ${pluginList.mkString(", ")}")
+ // Load executor plugins
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ private val executorPlugins =
+ Utils.loadExtensions(classOf[ExecutorPlugin], pluginList, conf)
+ executorPlugins.foreach(_.init())
+ if (pluginList != Nil) logDebug("Finished loading plugins")
--- End diff --
Same.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213003599
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
@tgravescs thoughts?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95567 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95567/testReport)** for PR 22192 at commit [`fa19ea8`](https://github.com/apache/spark/commit/fa19ea880f4fc7eafb736e372479d4df2bffb74b).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216106482
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -219,6 +239,18 @@ private[spark] class Executor(
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
threadPool.shutdown()
+
+ // Notify plugins that executor is shutting down so they can terminate cleanly
+ Utils.withContextClassLoader(replClassLoader) {
+ plugins.foreach(plugin => {
+ try {
+ plugin.shutdown()
+ } catch {
+ case e: Exception =>
+ logWarning(s"Plugin ${plugin.getClass} failed to shutdown: ${e.getMessage()}")
--- End diff --
Log the whole exception, not just the message.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95814 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95814/testReport)** for PR 22192 at commit [`cf30ed5`](https://github.com/apache/spark/commit/cf30ed52c0aefc1eda8efffc11e73cafc93a032d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95377 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95377/testReport)** for PR 22192 at commit [`284beaf`](https://github.com/apache/spark/commit/284beafa03010ccdacf6f905914fb91f340d09b6).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
These tests are failing locally for me, both with my code and without on a clean pull of master
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #96211 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96211/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95544/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216112535
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,122 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+public class ExecutorPluginSuite {
+ private static final String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private static final String testBadPluginName = TestBadShutdownPlugin.class.getName();
+ private static final String testPluginName = TestExecutorPlugin.class.getName();
+
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+
+ private JavaSparkContext sc;
+
+ @Before
+ public void setUp() {
+ sc = null;
+ numSuccessfulPlugins = 0;
+ numSuccessfulTerminations = 0;
+ }
+
+ @After
+ public void tearDown() {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set(EXECUTOR_PLUGIN_CONF_NAME, pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ // Load the sample TestExecutorPlugin, which will change the value of numSuccessfulPlugins
+ SparkConf conf = initializeSparkConf(testPluginName);
+ sc = new JavaSparkContext(conf);
+ assertEquals(1, numSuccessfulPlugins);
+ sc.stop();
+ sc = null;
+ assertEquals(1, numSuccessfulTerminations);
+ }
+
+ @Test
+ public void testAddMultiplePlugins() throws InterruptedException {
+ // Load the sample TestExecutorPlugin twice
+ SparkConf conf = initializeSparkConf(testPluginName + "," + testPluginName);
+ sc = new JavaSparkContext(conf);
+ assertEquals(2, numSuccessfulPlugins);
+ sc.stop();
+ sc = null;
+ assertEquals(2, numSuccessfulTerminations);
+ }
+
+ @Test
+ public void testPluginShutdownWithException() {
+ // Verify an exception in one plugin shutdown does not affect the others
+ String pluginNames = testPluginName + "," + testBadPluginName + "," + testPluginName;
+ SparkConf conf = initializeSparkConf(pluginNames);
+ sc = new JavaSparkContext(conf);
+ assertEquals(2, numSuccessfulPlugins);
--- End diff --
The bad plugin doesn't increment numSuccessfulPlugins, although in retrospect it's probably a good idea that it does...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #96212 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96212/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
I'm going to commit the fixes recommended by @vanzin soon. Then, I think the biggest remaining concern is the threads. I'm okay with adding a `start` and `stop`, but I'm curious as to whether those should potentially be in a separate Jira. This could potentially go into 2.4, and more methods could be added as we learn the use cases for plugins. My concern with adding them is that if it's decided they're obsolete or repetitive (in the case of `init` vs `start`), but people have already started using some plugins, it'd be annoying to take away the methods they expect to work. My best argument for adding the methods is we could try to ensure people conform to a similar structure if we have, say, the `init` function run in the same thread as the executor, and then once the executors have initialized we could create a thread that `start`s the plugins so really plugin writers wouldn't have to even worry about making their plugins async. Does anyone have strong opinions?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212746626
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -604,4 +604,15 @@ package object config {
.intConf
.checkValue(v => v > 0, "The max failures should be a positive value.")
.createWithDefault(40)
+
+ private[spark] val EXECUTOR_PLUGINS =
+ ConfigBuilder("spark.executor.plugins")
+ .internal()
--- End diff --
Not really internal, since the feature requires users to set it.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
If I want to add more testing plugins (i.e. to test that a plugin failing to shut down doesn't affect other plugins), is it stylistically ok for me to keep throwing them at the bottom of ExecutorPluginSuite? Or should I make a separate file (or multiple) for them?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95814/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95557 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95557/testReport)** for PR 22192 at commit [`a968b0a`](https://github.com/apache/spark/commit/a968b0ad23b4c3f1a21a4153416515f5025d4fe9).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213828422
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
@vanzin If I'm understanding correctly, in the ConfigBuilder I am to change `.createOptional` to `.createWithDefault(Nil)`. This gives the error:
`[error] /Users/nsheth/personal_fork/spark/core/src/main/scala/org/apache/spark/executor/Executor.scala:136: type mismatch;
[error] found : String
[error] required: Seq[String]
[error] Utils.loadExtensions(classOf[ExecutorPlugin], classes, conf)`
Granted I have no idea why it's able to cast a Nil to a string for this config.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22192
why do we merge a new API to branch-2.4, when RC1 is already out?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216049113
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -219,6 +236,13 @@ private[spark] class Executor(
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
threadPool.shutdown()
+
+ // Notify plugins that executor is shutting down so they can terminate cleanly
+ // oldContextClassLoader is redefined in case the class loader changed during execution
+ val oldContextClassLoader = Utils.getContextOrSparkClassLoader
+ Thread.currentThread.setContextClassLoader(replClassLoader)
+ plugins.foreach(_.shutdown())
--- End diff --
Just so Mridul's comment doesn't get lost: you need to `try` around the shutdown call.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214486292
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,114 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+ private JavaSparkContext sc;
+
+ private String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
--- End diff --
final static
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95810/
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212746543
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
Why a thread? This risks having the executor start doing things before plugins have had a chance of properly starting up.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95503 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95503/testReport)** for PR 22192 at commit [`2907c6b`](https://github.com/apache/spark/commit/2907c6b62495f8d25c0016883202239634685fec).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95915 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95915/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212746377
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
+ val executorPluginThread = new Thread {
+ override def run: Unit = {
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
+ Utils.loadExtensions(classOf[ExecutorPlugin], classes, conf)
+ }
+ }
+ }
+ executorPluginThread.start
--- End diff --
start()
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95559 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95559/testReport)** for PR 22192 at commit [`fa19ea8`](https://github.com/apache/spark/commit/fa19ea880f4fc7eafb736e372479d4df2bffb74b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95138/
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213142394
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
I guess it could be in the constructor; `Utils.loadExtensions` already provides a `SparkConf` to the constructor if one accepts it, which was the only thing I could think of.
I generally dislike plugin APIs that encourage initialization in the constructor, but here, other than maybe potentially some benefit for testing, I'm not seeing a lot of differences in not having the init method after all...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the issue:
https://github.com/apache/spark/pull/22192
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95138 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95138/testReport)** for PR 22192 at commit [`7c86fc5`](https://github.com/apache/spark/commit/7c86fc54c36954f1345eccc066873f7f90832657).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95136 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95136/testReport)** for PR 22192 at commit [`44454dd`](https://github.com/apache/spark/commit/44454dd586e35bdf16492c4a8969494bd3b7f8f5).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95503 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95503/testReport)** for PR 22192 at commit [`2907c6b`](https://github.com/apache/spark/commit/2907c6b62495f8d25c0016883202239634685fec).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
LGTM.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216209462
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -240,6 +240,19 @@ private[spark] object Utils extends Logging {
// scalastyle:on classforname
}
+ /**
+ * Run a segment of code using a different context class loader in the current thread
+ */
+ def withContextClassLoader[T](ctxClassLoader: ClassLoader)(fn: => T): T = {
+ val oldClassLoader = getContextOrSparkClassLoader
--- End diff --
Originally thought it might be a better idea since the spark executor uses that method when it runs but now that you mention it it could potentially change the behavior...I'll change it back
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95559/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95681/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95947 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95947/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212780252
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
--- End diff --
Is it alright to use Seq in a Java file? I couldn't find it in any other java files in Spark that aren't in a target/ directory. I don't think the compiler is getting mad when I import it but want to make sure I'm sticking with the usual style rules
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95557/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/22192
retest this please.
It's that old "java.lang.reflect.InvocationTargetException: null" error we've seen many times.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213819091
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
IMO we should do this in a different thread.
It will isolate the execution creation/instantiation from executor initialization.
Also, we should do this after `Executor` initialization has completed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
I don't think this test failure was caused by my changes, it failed a ML test in python due to a worker crashing. Couldn't find anything in the logs indicating that my changes led to this issue (also I don't think I changed any functionality from my previous commit, which passed all tests). Could someone 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213820297
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
`Utils.loadExtensions` expects `classes` to be a sequence of strings, it gets mad at me if I add `.createWithDefault(Nil)` to the config
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
Hi, just want to follow up on this and see if anyone has additional comments/questions/issues. Please feel free to ping me and I'll respond as soon as I can!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212745901
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,38 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * *hopefully* be able to keep compatability by providing default implementations for any methods
--- End diff --
Remove the *s around "hopefully".
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95136/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #96276 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96276/testReport)** for PR 22192 at commit [`f853fe5`](https://github.com/apache/spark/commit/f853fe5fd17a8bbed367ba93c3cddcb7e0e535f3).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
I tested @vanzin's suggestion of storing the classloader, changing it to replClassLoader, and changing back after init (and then changing again for shutdown). It's working well (and this time I made sure it doesn't just shut down immediately; I ran on a cluster and confirmed it runs the init code and creates a thread, the thread stays alive for the duration of the executor, and the shutdown code runs fully before exiting). While it looks a little weird, the code is synchronous and shouldn't run into an issue with the classloader being set to something it shouldn't be set to while executing the existing Spark code, and ensures the plugins initialize fully during executor initialization.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216210046
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,26 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val executorPlugins: Seq[ExecutorPlugin] = {
+ val pluginNames = conf.get(EXECUTOR_PLUGINS)
+ if (pluginNames.nonEmpty) {
+ logDebug(s"Initializing the following plugins: ${pluginNames.mkString(", ")}")
+
+ // Plugins need to load using a class loader that includes the executor's user classpath
+ val pluginList: Seq[ExecutorPlugin] =
+ Utils.withContextClassLoader(replClassLoader) {
+ val plugins = Utils.loadExtensions(classOf[ExecutorPlugin], pluginNames, conf)
+ plugins.foreach(_.init())
--- End diff --
I think that should be the right behavior, since if a plugin is faulty it might be a good idea for the user to handle that issue first. Since plugins are opt-in, a user providing one would expect it to function once it's included, and would probably want to know if it fails rather than run their entire job just to see that it didn't do what it was supposed to do
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by squito <gi...@git.apache.org>.
Github user squito commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213003400
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
--- End diff --
yeah I made the interface in java to make it easier for plugin-writers, since it didn't need scala at all. actually an empty trait also compiles to an empty interface in java with no references to scala, but I figured this would help remember that if the interface gets added to in the future.
I don't see any reason the test needs to be in java, but don't feel strongly either.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216048261
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,23 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val pluginList = conf.get(EXECUTOR_PLUGINS)
--- End diff --
Just noticed... all this code is in the constructor, so it would be better to not leave unnecessary fields around. Something like:
```
private val plugins: Seq[ExecutorPlugin] = {
// All this initialization code.
}
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95772 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95772/testReport)** for PR 22192 at commit [`b0be9e5`](https://github.com/apache/spark/commit/b0be9e575be25487e617e359d6bcfbef05a4bc8a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216116706
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -240,6 +240,19 @@ private[spark] object Utils extends Logging {
// scalastyle:on classforname
}
+ /**
+ * Run a segment of code using a different context class loader in the current thread
+ */
+ def withContextClassLoader[T](ctxClassLoader: ClassLoader)(fn: => T): T = {
+ val oldClassLoader = getContextOrSparkClassLoader
--- End diff --
Curious, why `getContextOrSparkClassLoader` and not just `Thread.getContextClassLoader` ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95814 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95814/testReport)** for PR 22192 at commit [`cf30ed5`](https://github.com/apache/spark/commit/cf30ed52c0aefc1eda8efffc11e73cafc93a032d).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r218865220
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,128 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+public class ExecutorPluginSuite {
+ private static final String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private static final String testBadPluginName = TestBadShutdownPlugin.class.getName();
+ private static final String testPluginName = TestExecutorPlugin.class.getName();
+
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+
+ private JavaSparkContext sc;
+
+ @Before
+ public void setUp() {
+ sc = null;
+ numSuccessfulPlugins = 0;
+ numSuccessfulTerminations = 0;
+ }
+
+ @After
+ public void tearDown() {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set(EXECUTOR_PLUGIN_CONF_NAME, pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ fail("No exception thrown for nonexistant plugin");
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ // Load the sample TestExecutorPlugin, which will change the value of numSuccessfulPlugins
+ SparkConf conf = initializeSparkConf(testPluginName);
+ sc = new JavaSparkContext(conf);
+ assertEquals(1, numSuccessfulPlugins);
+ sc.stop();
+ sc = null;
+ assertEquals(1, numSuccessfulTerminations);
+ }
+
+ @Test
+ public void testAddMultiplePlugins() throws InterruptedException {
--- End diff --
super nit: shall we test whether we can load multiple different plugins?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212747219
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ JavaSparkContext sc = null;
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ Assert.assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ } finally {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ JavaSparkContext sc = null;
+ numSuccessfulPlugins = 0;
+
+ // Load the sample TestExecutorPlugin, which will change the value of pluginExecutionSuccessful
+ SparkConf conf = initializeSparkConf("test.org.apache.spark.TestExecutorPlugin");
+
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ Assert.fail("Failed to start SparkContext with exception " + e.toString());
+ }
+
+ // Wait a moment since plugins run on separate threads
+ Thread.sleep(500);
+
+ Assert.assertEquals(1, numSuccessfulPlugins);
+
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+
+ @Test
+ public void testAddMultiplePlugins() throws InterruptedException {
+ JavaSparkContext sc = null;
+ numSuccessfulPlugins = 0;
+
+ // Load the sample TestExecutorPlugin twice
+ SparkConf conf = initializeSparkConf(
+ "test.org.apache.spark.TestExecutorPlugin,test.org.apache.spark.TestExecutorPlugin");
--- End diff --
indented too far
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212781171
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
--- End diff --
I had the same first response. The original executor plugin code was a class in Java, so I figured I should keep my test in Java as well (manually checked beforehand, couldn't find any instances of Java stuff being primarily tested in Scala so I figured it best not to do that). Perhaps it's for the plugin-writers to get to choose their plugin language, since it's easier to extend a Java interface in Scala, than to extend a Scala trait in Java? I'll leave it as is for the time being
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216106778
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,122 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+public class ExecutorPluginSuite {
+ private static final String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private static final String testBadPluginName = TestBadShutdownPlugin.class.getName();
+ private static final String testPluginName = TestExecutorPlugin.class.getName();
+
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+
+ private JavaSparkContext sc;
+
+ @Before
+ public void setUp() {
+ sc = null;
+ numSuccessfulPlugins = 0;
+ numSuccessfulTerminations = 0;
+ }
+
+ @After
+ public void tearDown() {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set(EXECUTOR_PLUGIN_CONF_NAME, pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ // Load the sample TestExecutorPlugin, which will change the value of numSuccessfulPlugins
+ SparkConf conf = initializeSparkConf(testPluginName);
+ sc = new JavaSparkContext(conf);
+ assertEquals(1, numSuccessfulPlugins);
+ sc.stop();
+ sc = null;
+ assertEquals(1, numSuccessfulTerminations);
+ }
+
+ @Test
+ public void testAddMultiplePlugins() throws InterruptedException {
+ // Load the sample TestExecutorPlugin twice
+ SparkConf conf = initializeSparkConf(testPluginName + "," + testPluginName);
+ sc = new JavaSparkContext(conf);
+ assertEquals(2, numSuccessfulPlugins);
+ sc.stop();
+ sc = null;
+ assertEquals(2, numSuccessfulTerminations);
+ }
+
+ @Test
+ public void testPluginShutdownWithException() {
+ // Verify an exception in one plugin shutdown does not affect the others
+ String pluginNames = testPluginName + "," + testBadPluginName + "," + testPluginName;
+ SparkConf conf = initializeSparkConf(pluginNames);
+ sc = new JavaSparkContext(conf);
+ assertEquals(2, numSuccessfulPlugins);
--- End diff --
Shouldn't this be 3?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #96212 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96212/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212777267
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
Originally was all in one thread, changed to separate based on this discussion: https://github.com/apache/spark/pull/21923#discussion_r206905166
I see the merits of both arguments, can change it based on the general consensus.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213792035
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
+ Utils.loadExtensions(classOf[ExecutorPlugin], classes, conf)
+ .foreach { _.init() }
--- End diff --
nit: `.foreach(_.init())` or `.foreach { p => p.init() }`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96211/
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216049652
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,116 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
--- End diff --
Unnecessary comment.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95559 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95559/testReport)** for PR 22192 at commit [`fa19ea8`](https://github.com/apache/spark/commit/fa19ea880f4fc7eafb736e372479d4df2bffb74b).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216116857
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,26 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val executorPlugins: Seq[ExecutorPlugin] = {
+ val pluginNames = conf.get(EXECUTOR_PLUGINS)
+ if (pluginNames.nonEmpty) {
+ logDebug(s"Initializing the following plugins: ${pluginNames.mkString(", ")}")
+
+ // Plugins need to load using a class loader that includes the executor's user classpath
+ val pluginList: Seq[ExecutorPlugin] =
+ Utils.withContextClassLoader(replClassLoader) {
+ val plugins = Utils.loadExtensions(classOf[ExecutorPlugin], pluginNames, conf)
+ plugins.foreach(_.init())
--- End diff --
Just to confirm - the behavior we are going for is, if `init()` fails, then the executor does not come up - right ?
This could be invalid plugins specified or even initialization of one or more plugin's failing.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213837582
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
Ah, whoops, misunderstood the value of `conf.get`. That works just fine
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213140764
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
Aside from semantics, would an `init` method be necessary instead of having the initialization logic be in the plugin's constructor? Since the class loader is going to call the constructor immediately, I figure having an `init` function would only really make a difference if we want to load the plugins right here, and then call `init` at a later point in the executor's creation. I can't think of any particular reason why we'd want to do that, unless there's specific executor structures that we want created prior to plugin initialization (although in that case we could also just move the plugin initialization further down)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95772/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
Can't be a coincidence...it's failing two pyspark streaming tests, test_binary_records_stream and test_text_file_stream (same two earlier, I misanalyzed it when I said it was ML stuff). I'm running them locally to try to reproduce, though not sure how they relate to my changes. Nonetheless other PRs are building fine right now so it must be something in my changes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
@mridulm I'm inclined to agree. I was doing some extra testing on separate thread and it seems to be performing well. I'll push this shortly. I'm going to also test setting the class loader and then changing back, since I'll always be partial towards synchronous operations, but separate thread is certainly seeming like a good policy given the pyspark failures.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214486314
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,114 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+ private JavaSparkContext sc;
+
+ private String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private String testPluginName = TestExecutorPlugin.class.getName();
--- End diff --
final static
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214418189
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,38 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * *hopefully* be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
--- End diff --
Both your arguments make sense, I can see a clear purpose for having a `stop`, given that we mostly care about it for successful runs without any weird early failures/terminations. The only case I can make for `start` (if we already have `init`) is if Spark itself calls the `start` methods from a separate thread instead of having plugin writers do it, but it might actually give them more flexibility to do it themselves. Having `start`/`stop` might be better than `init`/`stop`, though.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95947/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95376 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95376/testReport)** for PR 22192 at commit [`44aa634`](https://github.com/apache/spark/commit/44aa63442f7f2eb33d67fa1dedb64c9374ba0de0).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212747806
--- Diff: core/src/test/java/test/org/apache/spark/TestExecutorPlugin.java ---
@@ -0,0 +1,29 @@
+/*
+ * 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 test.org.apache.spark;
+
+import org.apache.spark.ExecutorPlugin;
+import org.apache.spark.ExecutorPluginSuite;
+
+// A test-only sample plugin, used by ExecutorPluginSuite to verify that
+// plugins are correctly loaded from the spark.executor.plugins conf
+public class TestExecutorPlugin implements ExecutorPlugin {
--- End diff --
Seems overkill to have this in a separate class in a completely separate package. It could even be a static class in the test suite.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216054443
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -219,6 +236,13 @@ private[spark] class Executor(
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
threadPool.shutdown()
+
+ // Notify plugins that executor is shutting down so they can terminate cleanly
+ // oldContextClassLoader is redefined in case the class loader changed during execution
+ val oldContextClassLoader = Utils.getContextOrSparkClassLoader
--- End diff --
Yes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216053464
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -219,6 +236,13 @@ private[spark] class Executor(
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
threadPool.shutdown()
+
+ // Notify plugins that executor is shutting down so they can terminate cleanly
+ // oldContextClassLoader is redefined in case the class loader changed during execution
+ val oldContextClassLoader = Utils.getContextOrSparkClassLoader
--- End diff --
Do you mean make a function that does something like
`Utils.withClassLoader(replClassLoader) {
load plugins
}` ?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95810 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95810/testReport)** for PR 22192 at commit [`10481f0`](https://github.com/apache/spark/commit/10481f043639745eb4db24636febcbe732be9921).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
I feel like it should be unrelated as well. It's strange that I failed tests in the same suite twice in a row, and that no other recent build has failed that suite, but I tried running locally on a mac and a linux vm and couldn't reproduce it so...fingers crossed
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212777617
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,38 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * *hopefully* be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
--- End diff --
Pinging @squito for this one, not sure if he specifically wanted an empty API to start (and have plugins run to their own completion or until the JVM cleans them up) or was considering adding methods later
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
merging to master / 2.4.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216049530
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -219,6 +236,13 @@ private[spark] class Executor(
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
threadPool.shutdown()
+
+ // Notify plugins that executor is shutting down so they can terminate cleanly
+ // oldContextClassLoader is redefined in case the class loader changed during execution
+ val oldContextClassLoader = Utils.getContextOrSparkClassLoader
--- End diff --
I think it's worth it to add a method in `Utils.scala` to do the class loader thingy. This code pattern is used in a bunch of places, it would be nice to make it easier (and later change existing code to use the new method).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216116705
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -68,7 +68,9 @@ public void testPluginClassDoesNotExist() {
} catch (Exception e) {
// We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ return;
--- End diff --
You can avoid this by having the `fail` inside the `try` block.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
^This commit is to show what that looks like. I'm still looking into how to properly barrier the plugins after calling `init` from a new thread (since `join`ing the thread requires the thread to die first, at which point it can no longer listen for executor `stop`ing)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95138 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95138/testReport)** for PR 22192 at commit [`7c86fc5`](https://github.com/apache/spark/commit/7c86fc54c36954f1345eccc066873f7f90832657).
* This patch **fails due to an unknown error code, -9**.
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95377 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95377/testReport)** for PR 22192 at commit [`284beaf`](https://github.com/apache/spark/commit/284beafa03010ccdacf6f905914fb91f340d09b6).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216051495
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,116 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ private static final String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private static final String testPluginName = TestExecutorPlugin.class.getName();
+
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+
+ private JavaSparkContext sc;
+
+ @Before
+ public void setUp() {
+ sc = null;
+ numSuccessfulPlugins = 0;
+ numSuccessfulTerminations = 0;
+ }
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set(EXECUTOR_PLUGIN_CONF_NAME, pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ } finally {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ // Load the sample TestExecutorPlugin, which will change the value of numSuccessfulPlugins
+ SparkConf conf = initializeSparkConf(testPluginName);
+
+ try {
+ sc = new JavaSparkContext(conf);
+ assertEquals(1, numSuccessfulPlugins);
+ } catch (Exception e) {
+ fail("Failed to start SparkContext with exception " + e.toString());
+ } finally {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ assertEquals(1, numSuccessfulTerminations);
+ }
+ }
+ }
+
+ @Test
+ public void testAddMultiplePlugins() throws InterruptedException {
+ // Load the sample TestExecutorPlugin twice
+ SparkConf conf = initializeSparkConf(testPluginName + "," + testPluginName);
+
+ try {
--- End diff --
Better:
```
sc = new JavaSparkContext(conf);
assertEquals(2, numSuccessfulPlugins);
sc.stop();
sc = null;
assertEquals(2, numSuccessfulTerminations);
```
Then you don't need to handle exceptions. You should have an `@After` method to clean up `sc` if it's still around, and then the other test can also be simplified.
Catching exceptions in tests is weird, unless you're explicitly testing for them to be thrown.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95503/
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216106374
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,26 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val plugins: Seq[ExecutorPlugin] = {
+ val pluginNames = conf.get(EXECUTOR_PLUGINS)
+ if (pluginNames.nonEmpty) {
+ logDebug(s"Initializing the following plugins: ${pluginNames.mkString(", ")}")
+
+ // Before loading the executor plugins, we need to temporarily change the class loader
--- End diff --
This comment is actually not currently accurate unless you're running YARN. In this context, it should mention the "executor's user classpath", not how it's formed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215372559
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,50 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * hopefully be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all Spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
+
+ /**
+ * Initialization method that will be called during executor startup, in the same thread as
--- End diff --
Since this is a new public API and thus this is public documentation, follow javadoc rules:
- first sentence should be short
- break things into paragraphs as needed (separated with `<p>`).
e.g.
```
Callback for initializing an executor plugin. This is called synchronously from the executor initialization thread.
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95862 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95862/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
* This patch **fails due to an unknown error code, -9**.
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213791918
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
This becomes more readable if the config has a `Nil` default value instead of being optional.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216106455
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -219,6 +239,18 @@ private[spark] class Executor(
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
threadPool.shutdown()
+
+ // Notify plugins that executor is shutting down so they can terminate cleanly
+ Utils.withContextClassLoader(replClassLoader) {
+ plugins.foreach(plugin => {
--- End diff --
`.foreach { plugin =>`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95544 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95544/testReport)** for PR 22192 at commit [`2907c6b`](https://github.com/apache/spark/commit/2907c6b62495f8d25c0016883202239634685fec).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95376 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95376/testReport)** for PR 22192 at commit [`44aa634`](https://github.com/apache/spark/commit/44aa63442f7f2eb33d67fa1dedb64c9374ba0de0).
* This patch **fails Java 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 pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213792638
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,114 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+ private JavaSparkContext sc;
+
+ private String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private String testPluginName = "org.apache.spark.ExecutorPluginSuite$TestExecutorPlugin";
+
+ @Before
+ public void setUp() {
+ sc = null;
+ numSuccessfulPlugins = 0;
+ }
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set(EXECUTOR_PLUGIN_CONF_NAME, pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ } finally {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ // Load the sample TestExecutorPlugin, which will change the value of pluginExecutionSuccessful
+ SparkConf conf = initializeSparkConf(testPluginName);
+
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ fail("Failed to start SparkContext with exception " + e.toString());
+ }
+
+ // Wait a moment since plugins run on separate threads
+ Thread.sleep(500);
+
+ assertEquals(1, numSuccessfulPlugins);
+
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+
+ @Test
+ public void testAddMultiplePlugins() throws InterruptedException {
+ // Load the sample TestExecutorPlugin twice
+ SparkConf conf = initializeSparkConf(testPluginName + "," + testPluginName);
+
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ fail("Failed to start SparkContext with exception " + e.toString());
+ }
+
+ // Wait a moment since plugins run on a separate thread
+ Thread.sleep(500);
+
+ assertEquals(2, numSuccessfulPlugins);
+
+ if (sc != null) {
--- End diff --
This needs to be in a finally block. Same in the other test. There's a base class in Scala that helps here, but don't remember one for Java...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216106676
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,122 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+public class ExecutorPluginSuite {
+ private static final String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private static final String testBadPluginName = TestBadShutdownPlugin.class.getName();
+ private static final String testPluginName = TestExecutorPlugin.class.getName();
+
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+
+ private JavaSparkContext sc;
+
+ @Before
+ public void setUp() {
+ sc = null;
+ numSuccessfulPlugins = 0;
+ numSuccessfulTerminations = 0;
+ }
+
+ @After
+ public void tearDown() {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set(EXECUTOR_PLUGIN_CONF_NAME, pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
--- End diff --
You should fail the test if an exception is not thrown here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95887 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95887/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95565/
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214354883
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,11 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load executor plugins
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ Utils.loadExtensions(classOf[ExecutorPlugin], conf.get(EXECUTOR_PLUGINS), conf)
--- End diff --
not a big deal but any reason we did this in between setting up the classloaders, perhaps move down a few lines just to keep all the classloader stuff together.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95182/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95887/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215750952
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -218,6 +244,8 @@ private[spark] class Executor(
env.metricsSystem.report()
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
+ executorPluginThread.interrupt()
+ executorPluginThread.join()
--- End diff --
We should be careful about semantics of `interrupt()` when overriding it - here we are using it to trigger something in addition to thread interruption.
For example, the actual shutdown of plugin's is getting triggered in this caller thread - and not in plugin init thread : which can cause issues since plugin's are using a a different thread ctx classloader.
This usage is actually a bit confusing.
A better option, if we want to follow current design, would be:
* Subclass Thread - expose methods to trigger state changes.
* Once init completes, wait+notify/condition wait until plugin shutdown is required.
* On stop(), invoke plugin shutdown in plugin thread (so that right classloader is in use).
IIRC VM shutdown results in `executor.stop` - and so plugin shutdown should get invoked even in that case.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214485930
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,15 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val pluginList = conf.get(EXECUTOR_PLUGINS)
+ if (pluginList != Nil) logDebug(s"Loading the following plugins: ${pluginList.mkString(", ")}")
--- End diff --
`pluginList.nonEmpty`
Also we generally use the full syntax with braces, unless it's a one line `if..else`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95584/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
> I'll change the config documentation to specify YARN only, hopefully it's not a huge issue.
I don't think you're really understanding what I'm saying above. I'm not saying this feature only works with YARN. I'm saying that distributing the plugins with `--jars` only works with YARN. And that if there was user-documentation for this feature - which there isn't - then it would have to explain how the jar is expected to be made available to the executors.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95182 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95182/testReport)** for PR 22192 at commit [`7c86fc5`](https://github.com/apache/spark/commit/7c86fc54c36954f1345eccc066873f7f90832657).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212780758
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
--- End diff --
Ah, crap, this is Java... any specific reason to do this in Java? Otherwise it's probably fine the way it is.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213818362
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,38 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * *hopefully* be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
--- End diff --
I agree, explicit start/stop is a good idea; it also makes the lifecycle more concrete.
With the explicit caveat that class loading need not result in instantiation, and object creation need not necessarily result in a start(), or a `stop()` being invoked for every `start()` (in case of VM failures).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95377/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by squito <gi...@git.apache.org>.
Github user squito commented on the issue:
https://github.com/apache/spark/pull/22192
retest this please.
I took a look at the failures, pretty certain its unrelated, and I filed https://issues.apache.org/jira/browse/SPARK-25400 to increase the timeouts in one of those failures.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95915/
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214353529
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,38 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * *hopefully* be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
--- End diff --
I definitely think a stop would be good. Since we don't have this currently in a separate thread not sure how start is different from init? I guess if we do it later or something but think that could be added later unless someone has explicitly use case
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212747362
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
--- End diff --
I personally prefer `import static org.junit.Assert.*;`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213045752
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
I guess it does depend on what the intended use is here. If we have it in the same thread it has the issue that it could block the executor or take to long and things start timing out. It can have more direct impact on the executor code itself, where as a separate thread isolates it more. But like you say if its not here and we don't wait for it then we could have order issue if certain plugins have to be initialized before other things happen. I can see both arguments as well. So perhaps the api needs an init type function that can be called more inline with a timeout to prevent from taking to long and the main part of the plugin called in a separate thread?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
> I'm looking up other spark-submit options that can be used to provide the jar to other nodes
I don't think there is anything like that currently; so using `--jars` for this will only work in YARN mode. But that can be just a documentation thingy, when/if this feature is documented.
(It may work with kubernetes too but haven't really tried that out since 2.3.)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
This was discussed way before RC1 was cut. Not adding this to 2.4 makes the timeline for this API to be out way too long in the future, and we want people to try it out.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
it's ok to keep them in the same suite where they are used.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95567 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95567/testReport)** for PR 22192 at commit [`fa19ea8`](https://github.com/apache/spark/commit/fa19ea880f4fc7eafb736e372479d4df2bffb74b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95584 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95584/testReport)** for PR 22192 at commit [`5a2852f`](https://github.com/apache/spark/commit/5a2852fb587278ab6a81c94de4226306a2e0da70).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95584 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95584/testReport)** for PR 22192 at commit [`5a2852f`](https://github.com/apache/spark/commit/5a2852fb587278ab6a81c94de4226306a2e0da70).
* This patch **fails due to an unknown error code, -9**.
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
You're right, I ran in local-cluster and it exited very quickly citing executors shutting down after not being able to find my test plugin. Although, the logs say that it does use a CoarseGrainedExecutorBackend:
`18/09/05 12:03:20 INFO CoarseGrainedExecutorBackend: Connecting to driver: spark://CoarseGrainedScheduler@nihar-xxx:45767`
unless you mean Yarn only uses that one command line option. I'm looking into how it reacts in regular standalone mode with different thread counts. Thanks for pointing this out!
I'm looking up other spark-submit options that can be used to provide the jar to other nodes, but I'm not super hopeful it's going to work out. If it indeed doesn't, I'll start exploring other options once I figure out why the pyspark tests are failing
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95681 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95681/testReport)** for PR 22192 at commit [`5a2852f`](https://github.com/apache/spark/commit/5a2852fb587278ab6a81c94de4226306a2e0da70).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #96276 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96276/testReport)** for PR 22192 at commit [`f853fe5`](https://github.com/apache/spark/commit/f853fe5fd17a8bbed367ba93c3cddcb7e0e535f3).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95182 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95182/testReport)** for PR 22192 at commit [`7c86fc5`](https://github.com/apache/spark/commit/7c86fc54c36954f1345eccc066873f7f90832657).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #4333 has finished](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4333/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95772 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95772/testReport)** for PR 22192 at commit [`b0be9e5`](https://github.com/apache/spark/commit/b0be9e575be25487e617e359d6bcfbef05a4bc8a).
* This patch **fails Java 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95565 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95565/testReport)** for PR 22192 at commit [`fa19ea8`](https://github.com/apache/spark/commit/fa19ea880f4fc7eafb736e372479d4df2bffb74b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213821565
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,114 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+ private JavaSparkContext sc;
+
+ private String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private String testPluginName = "org.apache.spark.ExecutorPluginSuite$TestExecutorPlugin";
+
+ @Before
+ public void setUp() {
+ sc = null;
+ numSuccessfulPlugins = 0;
+ }
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set(EXECUTOR_PLUGIN_CONF_NAME, pluginNames);
+ }
+
+ @Test
+ public void testPluginClassDoesNotExist() {
+ SparkConf conf = initializeSparkConf("nonexistant.plugin");
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ // We cannot catch ClassNotFoundException directly because Java doesn't think it'll be thrown
+ assertTrue(e.toString().startsWith("java.lang.ClassNotFoundException"));
+ } finally {
+ if (sc != null) {
+ sc.stop();
+ sc = null;
+ }
+ }
+ }
+
+ @Test
+ public void testAddPlugin() throws InterruptedException {
+ // Load the sample TestExecutorPlugin, which will change the value of pluginExecutionSuccessful
+ SparkConf conf = initializeSparkConf(testPluginName);
+
+ try {
+ sc = new JavaSparkContext(conf);
+ } catch (Exception e) {
+ fail("Failed to start SparkContext with exception " + e.toString());
+ }
+
+ // Wait a moment since plugins run on separate threads
+ Thread.sleep(500);
+
+ assertEquals(1, numSuccessfulPlugins);
--- End diff --
After introducing an explicit start(), we can replace the `sleep` with a timed wait on a condition.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
Please remove all of the PR template text from the description.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on the issue:
https://github.com/apache/spark/pull/22192
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213106129
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
I suggested an `init` method in another comment already, and I think that should be enough, if coupled with documentation saying that method is calling inline with the initialization of the executor, and should avoid blocking it, doing any expensive initialization asynchronously.
We could try to be fancy and add both "init()" and "initAsync" or something to make it easier for plugin writers, but that sounds like a small gain for the extra complexity in the interface.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212747168
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
--- End diff --
Use `.set(CONSTANT, Seq(...))`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by felixcheung <gi...@git.apache.org>.
Github user felixcheung commented on the issue:
https://github.com/apache/spark/pull/22192
Jenkins, 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95810 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95810/testReport)** for PR 22192 at commit [`10481f0`](https://github.com/apache/spark/commit/10481f043639745eb4db24636febcbe732be9921).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95862/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
I'll change the config documentation to specify YARN only, hopefully it's not a huge issue.
It seems like the line `Thread.currentThread().setContextClassLoader(replClassLoader)` is causing the pyspark failures, they pass when I remove it. I'm looking at the test cases but I really don't see how this is affecting them...it seems that in both test cases, the DStreams monitor a directory but don't pick up the changes they're supposed to, and just time out. I checked that I can bypass this issue by changing back to having the plugins loaded on a separate thread (and setting that thread's contextClassLoader instead of the current thread) and it passes tests and continues to work. That said this issue does seem to be indicative of some problem in pyspark streaming
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213831980
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
@mridulm We've had some back and forth on that, open to changing it if people agree it should be one way over another. Just checking, have you looked at the earlier conversation at https://github.com/apache/spark/pull/22192#discussion_r212746543? There's also one on the old PR for the same topic.
As for moving plugins for after executors have initialized, I see no problem with that. I don't think it should make a huge difference provided either we do change to separate thread, or we keep it in the same thread but writers respect not making the initialization blocking and computation heavy. I'll see if there's a more natural place to move it
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212746238
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,38 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * *hopefully* be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
--- End diff --
I think it would be better to have explicit `start()` and `stop()` methods in this interface. Otherwise, especially in the latter case, there is no way for the plugin to know when to do cleanup.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215760787
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -218,6 +244,8 @@ private[spark] class Executor(
env.metricsSystem.report()
heartbeater.shutdown()
heartbeater.awaitTermination(10, TimeUnit.SECONDS)
+ executorPluginThread.interrupt()
+ executorPluginThread.join()
--- End diff --
Oops, yeah, my bad. I believe the current implementation also terminates the plugins immediately...
For your recommendation, would the thread that initializes the plugins also be undergoing the conditional wait? If so, would that mean I cannot join on this thread right after initialization of plugins (from your earlier comment) since the thread would stay alive to wait for the shutdown condition?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95887 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95887/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96212/
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216106874
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -240,6 +240,16 @@ private[spark] object Utils extends Logging {
// scalastyle:on classforname
}
+ /**
+ * Run a segment of code using a different context class loader in the current thread
+ */
+ def withContextClassLoader(ctxClassLoader: ClassLoader)(fn: => Unit): Unit = {
--- End diff --
Better to make this method generic (i.e. make the return a parameterized type `T`).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216118263
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,26 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val executorPlugins: Seq[ExecutorPlugin] = {
+ val pluginNames = conf.get(EXECUTOR_PLUGINS)
+ if (pluginNames.nonEmpty) {
+ logDebug(s"Initializing the following plugins: ${pluginNames.mkString(", ")}")
+
+ // Plugins need to load using a class loader that includes the executor's user classpath
+ val pluginList: Seq[ExecutorPlugin] =
+ Utils.withContextClassLoader(replClassLoader) {
+ val plugins = Utils.loadExtensions(classOf[ExecutorPlugin], pluginNames, conf)
+ plugins.foreach(_.init())
--- End diff --
On second thoughts, I am not sure if the latter should fail the executor ... for example if plugin is unable to write a file, etc
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216105914
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,59 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * hopefully be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all Spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
+
+ /**
+ * Initialize the executor plugin.
+ *
+ * <p>Each executor will, during its initialization, invoke this method on each
+ * plugin provided in the spark.executor.plugins configuration. These invocations
+ * will occur asynchronously from the executor initialization, but synchronously
--- End diff --
This is not true anymore, is it?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95567/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the issue:
https://github.com/apache/spark/pull/22192
@vanzin @NiharS I am uncomfortable with that change as well - which is why I wanted the initialization to be pushed into a separate thread (and then join) - if we really need to set the context classloader.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22192
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95544 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95544/testReport)** for PR 22192 at commit [`2907c6b`](https://github.com/apache/spark/commit/2907c6b62495f8d25c0016883202239634685fec).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212779435
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
--- End diff --
`.set(EXECUTOR_PLUGINS, Seq("class1", "class2"))`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213821674
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
Gets mad how? A default of `Nil` should still create a config constant that returns a `Seq[String]`, right?
(I know I added the other configs that use this and they are optional instead of having a default value, but I'm kinda wondering what is the issue you're referring to.)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214353202
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,39 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * hopefully be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all Spark releases.
+ *
+ * Spark does nothing to verify the plugin is doing legitimate things, or to manage the resources
+ * it uses. A plugin acquires the same privileges as the user running the task. A bad plugin
+ * could also intefere with task execution and make the executor fail in unexpected ways.
+ */
+@DeveloperApi
+public interface ExecutorPlugin {
+ default void init() {}
--- End diff --
javadoc for init
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95862 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95862/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213832471
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
+ Thread.currentThread().setContextClassLoader(replClassLoader)
+ conf.get(EXECUTOR_PLUGINS).foreach { classes =>
--- End diff --
It's not just changing the config. You'd need to change the call too. e.g.
Utils.loadExtensions(classOf[ExecutorPlugin], conf.get(EXECUTOR_PLUGINS), conf)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215749405
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,32 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val pluginList = conf.get(EXECUTOR_PLUGINS)
+ if (pluginList.nonEmpty) {
+ logDebug(s"Initializing the following plugins: ${pluginList.mkString(", ")}")
+ }
+
+ val executorPluginThread = new Thread {
+ var plugins: Seq[ExecutorPlugin] = Nil
+
+ override def run(): Unit = {
+ plugins = Utils.loadExtensions(classOf[ExecutorPlugin], pluginList, conf)
+ plugins.foreach(_.init())
+ }
+
+ override def interrupt(): Unit = {
+ plugins.foreach(_.shutdown())
+ super.interrupt()
--- End diff --
super.interrupt in try/finally.
Also would be good to isolate the shutdown of each plugin from others (so that an exception thrown in one plugin does not prevent another plugin from shutting down)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95136 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95136/testReport)** for PR 22192 at commit [`44454dd`](https://github.com/apache/spark/commit/44454dd586e35bdf16492c4a8969494bd3b7f8f5).
* This patch **fails Java style tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* ` .doc(\"Comma-separated list of class names for \"plugins\" implementing \" +`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95915 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95915/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
* This patch **fails due to an unknown error code, -9**.
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213150133
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,16 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // One thread will handle loading all of the plugins on this executor
--- End diff --
That does make sense. While I did say "aside from semantics", semantics is a good reason to include it. Especially since it'll be harder to get plugin writers to adopt an `init` function later. I'll make the other changes and make sure the tests still pass, if anyone does feel strongly (or even weakly) on one way over another I don't think there's much harm in either approach.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95806/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95681 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95681/testReport)** for PR 22192 at commit [`5a2852f`](https://github.com/apache/spark/commit/5a2852fb587278ab6a81c94de4226306a2e0da70).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212745955
--- Diff: core/src/main/java/org/apache/spark/ExecutorPlugin.java ---
@@ -0,0 +1,38 @@
+/*
+ * 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;
+
+import org.apache.spark.annotation.DeveloperApi;
+
+/**
+ * A plugin which can be automaticaly instantiated within each Spark executor. Users can specify
+ * plugins which should be created with the "spark.executor.plugins" configuration. An instance
+ * of each plugin will be created for every executor, including those created by dynamic allocation,
+ * before the executor starts running any tasks.
+ *
+ * The specific api exposed to the end users still considered to be very unstable. We will
+ * *hopefully* be able to keep compatability by providing default implementations for any methods
+ * added, but make no guarantees this will always be possible across all spark releases.
--- End diff --
Spark
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216116693
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -242,14 +242,17 @@ private[spark] class Executor(
// Notify plugins that executor is shutting down so they can terminate cleanly
Utils.withContextClassLoader(replClassLoader) {
- plugins.foreach(plugin => {
+ executorPlugins.foreach { plugin =>
try {
plugin.shutdown()
} catch {
case e: Exception =>
- logWarning(s"Plugin ${plugin.getClass} failed to shutdown: ${e.getMessage()}")
+ logWarning(
+ s"""Plugin ${plugin.getClass().getCanonicalName()} failed to shutdown:
+ |${e.toString}
--- End diff --
You can just use `logWarning("Msg", e)` to log the whole thing...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #96211 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96211/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96276/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
(to my understanding this issue isn't part of my change, I checked other pulls that had this error around the same time and those also had similar outputs and lack of specific errors)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214486385
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,114 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+ // Static value modified by testing plugin to verify plugins shut down properly.
+ public static int numSuccessfulTerminations = 0;
+ private JavaSparkContext sc;
--- End diff --
Move after the variables below (which should be "final static").
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r212778867
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,104 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;
+import org.junit.Test;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+
+ private SparkConf initializeSparkConf(String pluginNames) {
+ return new SparkConf()
+ .setMaster("local")
+ .setAppName("test")
+ .set("spark.executor.plugins", pluginNames);
--- End diff --
Sorry, could you explain this for me? Do you mean instead of `.set("spark.executor.plugins", pluginNames)` I should a) have a variable storing the config name and b) pass the plugin names to initializeSparkConf as a list, and have it as `.set(EXECUTOR_PLUGIN_CONF_NAME, String.join(",", pluginNames)`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95557 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95557/testReport)** for PR 22192 at commit [`a968b0a`](https://github.com/apache/spark/commit/a968b0ad23b4c3f1a21a4153416515f5025d4fe9).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by NiharS <gi...@git.apache.org>.
Github user NiharS commented on the issue:
https://github.com/apache/spark/pull/22192
I believe this is another glitch, not from my changes. Could someone 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213792183
--- Diff: core/src/test/java/org/apache/spark/ExecutorPluginSuite.java ---
@@ -0,0 +1,114 @@
+/*
+ * 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;
+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Before;
+import org.junit.Test;
+
+import static org.junit.Assert.*;
+
+// Tests loading plugins into executors
+public class ExecutorPluginSuite {
+ // Static value modified by testing plugin to ensure plugin loaded correctly.
+ public static int numSuccessfulPlugins = 0;
+ private JavaSparkContext sc;
+
+ private String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+ private String testPluginName = "org.apache.spark.ExecutorPluginSuite$TestExecutorPlugin";
--- End diff --
`TestExecutorPlugin.class.getName()`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r215749556
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,32 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val pluginList = conf.get(EXECUTOR_PLUGINS)
+ if (pluginList.nonEmpty) {
+ logDebug(s"Initializing the following plugins: ${pluginList.mkString(", ")}")
+ }
+
+ val executorPluginThread = new Thread {
+ var plugins: Seq[ExecutorPlugin] = Nil
+
+ override def run(): Unit = {
+ plugins = Utils.loadExtensions(classOf[ExecutorPlugin], pluginList, conf)
+ plugins.foreach(_.init())
+ }
+
+ override def interrupt(): Unit = {
+ plugins.foreach(_.shutdown())
+ super.interrupt()
+ }
+ }
+
+ executorPluginThread.setContextClassLoader(replClassLoader)
+ executorPluginThread.start()
+
+ if (pluginList.nonEmpty) {
+ logDebug("Finished initializing plugins")
+ }
+
--- End diff --
We should wait for plugin's to finish initialization (join() on thread) before moving on (particularly if we are emitting a message to indicate the same).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #4333 has started](https://amplab.cs.berkeley.edu/jenkins/job/NewSparkPullRequestBuilder/4333/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r213790939
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,14 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load plugins in the current thread, they are expected to not block.
+ // Heavy computation in plugin initialization should be done async.
--- End diff --
This comment makes more sense in the API doc, not here.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22192
**[Test build #95947 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95947/testReport)** for PR 22192 at commit [`447c5e5`](https://github.com/apache/spark/commit/447c5e5974ca2a176026e63518a7a6cf29b78008).
* 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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214354153
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -130,6 +130,11 @@ private[spark] class Executor(
private val urlClassLoader = createClassLoader()
private val replClassLoader = addReplClassLoaderIfNeeded(urlClassLoader)
+ // Load executor plugins
--- End diff --
Might be useful to have logDebug before and after these calls so if necessary we can see it trying to load plugins and then when its finished. Might help more easily find bad plugins. If others think its overkill I'm ok leaving it out though
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r216106909
--- Diff: core/src/main/scala/org/apache/spark/util/Utils.scala ---
@@ -240,6 +240,16 @@ private[spark] object Utils extends Logging {
// scalastyle:on classforname
}
+ /**
+ * Run a segment of code using a different context class loader in the current thread
+ */
+ def withContextClassLoader(ctxClassLoader: ClassLoader)(fn: => Unit): Unit = {
+ val oldClassLoader = getContextOrSparkClassLoader
+ Thread.currentThread().setContextClassLoader(ctxClassLoader)
+ fn
+ Thread.currentThread().setContextClassLoader(oldClassLoader)
--- End diff --
This has to be in a `finally`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22192: [SPARK-24918][Core] Executor Plugin API
Posted by vanzin <gi...@git.apache.org>.
Github user vanzin commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r214486250
--- Diff: core/src/main/scala/org/apache/spark/internal/config/package.scala ---
@@ -604,4 +604,14 @@ package object config {
.intConf
.checkValue(v => v > 0, "The max failures should be a positive value.")
.createWithDefault(40)
+
+ private[spark] val EXECUTOR_PLUGINS =
+ ConfigBuilder("spark.executor.plugins")
+ .doc("Comma-separated list of class names for \"plugins\" implementing " +
+ "org.apache.spark.ExecutorPlugin. Plugins have the same privileges as any task " +
+ "in a spark executor. They can also interfere with task execution and fail in " +
--- End diff --
Spark
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22192: [SPARK-24918][Core] Executor Plugin API
Posted by bersprockets <gi...@git.apache.org>.
Github user bersprockets commented on the issue:
https://github.com/apache/spark/pull/22192
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 #22192: [SPARK-24918][Core] Executor Plugin API
Posted by jiangxb1987 <gi...@git.apache.org>.
Github user jiangxb1987 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22192#discussion_r218861435
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -136,6 +136,26 @@ private[spark] class Executor(
// for fetching remote cached RDD blocks, so need to make sure it uses the right classloader too.
env.serializerManager.setDefaultClassLoader(replClassLoader)
+ private val executorPlugins: Seq[ExecutorPlugin] = {
+ val pluginNames = conf.get(EXECUTOR_PLUGINS)
+ if (pluginNames.nonEmpty) {
+ logDebug(s"Initializing the following plugins: ${pluginNames.mkString(", ")}")
+
+ // Plugins need to load using a class loader that includes the executor's user classpath
+ val pluginList: Seq[ExecutorPlugin] =
+ Utils.withContextClassLoader(replClassLoader) {
+ val plugins = Utils.loadExtensions(classOf[ExecutorPlugin], pluginNames, conf)
+ plugins.foreach(_.init())
--- End diff --
nit: Might be good to log whether each `plugin.init()` succeeded.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org