[SPARK-24918][Core] Executor Plugin API #22192

NiharS · 2018-08-22T23:37:44Z

What changes were proposed in this pull request?

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.

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.

This commit adds testing and moves the plugin initialization to a separate thread.

vanzin · 2018-08-22T23:42:10Z

ok to test

vanzin · 2018-08-22T23:42:25Z

Please remove all of the PR template text from the description.

felixcheung · 2018-08-23T03:50:25Z

Jenkins, ok to test

SparkQA · 2018-08-23T04:11:19Z

Test build #95136 has finished for PR 22192 at commit 44454dd.

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 \" +

SparkQA · 2018-08-23T07:05:01Z

Test build #95138 has finished for PR 22192 at commit 7c86fc5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

NiharS · 2018-08-23T19:00:18Z

retest this please

NiharS · 2018-08-23T19:00:53Z

(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). Also no idea if jenkins will listen to me yet

vanzin · 2018-08-23T19:51:09Z

retest this please

SparkQA · 2018-08-24T00:13:12Z

Test build #95182 has finished for PR 22192 at commit 7c86fc5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-08-24T20:24:49Z

core/src/main/java/org/apache/spark/ExecutorPlugin.java

+ * 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


Remove the *s around "hopefully".

vanzin · 2018-08-24T20:25:04Z

core/src/main/java/org/apache/spark/ExecutorPlugin.java

+ *
+ * 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.


core/src/main/java/org/apache/spark/ExecutorPlugin.java

vanzin · 2018-08-24T20:27:05Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+      }
+    }
+  }
+  executorPluginThread.start


vanzin · 2018-08-24T20:27:43Z

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


Why a thread? This risks having the executor start doing things before plugins have had a chance of properly starting up.

Originally was all in one thread, changed to separate based on this discussion: #21923 (comment)

I see the merits of both arguments, can change it based on the general consensus.

@tgravescs thoughts?

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?

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.

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)

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...

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.

EDIT: one of those changes is changing back to plugin construction happening in the current thread, it sounds like that's the consensus here

vanzin · 2018-08-24T20:30:43Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+    return new SparkConf()
+        .setMaster("local")
+        .setAppName("test")
+        .set("spark.executor.plugins", pluginNames);


Use .set(CONSTANT, Seq(...))

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)?

.set(EXECUTOR_PLUGINS, Seq("class1", "class2"))

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

Ah, crap, this is Java... any specific reason to do this in Java? Otherwise it's probably fine the way it is.

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

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.

vanzin · 2018-08-24T20:30:56Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+
+    // Load the sample TestExecutorPlugin twice
+    SparkConf conf = initializeSparkConf(
+            "test.org.apache.spark.TestExecutorPlugin,test.org.apache.spark.TestExecutorPlugin");


indented too far

vanzin · 2018-08-24T20:31:34Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+
+import org.apache.spark.api.java.JavaSparkContext;
+
+import org.junit.Assert;


I personally prefer import static org.junit.Assert.*;.

vanzin · 2018-08-24T20:32:36Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+  @Test
+  public void testAddPlugin() throws InterruptedException {
+    JavaSparkContext sc = null;
+    numSuccessfulPlugins = 0;


I'd do this in a @Before method.

vanzin · 2018-08-24T20:33:22Z

core/src/test/java/test/org/apache/spark/TestExecutorPlugin.java

+
+// 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 {


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.

…ic class in main testing file

SparkQA · 2018-08-28T18:45:36Z

Test build #95376 has finished for PR 22192 at commit 44aa634.

This patch fails Java style tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-08-29T01:10:52Z

Test build #95377 has finished for PR 22192 at commit 284beaf.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

NiharS · 2018-08-29T18:37:36Z

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?

vanzin · 2018-08-29T18:40:29Z

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.


This comment makes more sense in the API doc, not here.

vanzin · 2018-08-29T18:43:46Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+  // 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 =>


This becomes more readable if the config has a Nil default value instead of being optional.

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.

Utils.loadExtensions expects classes to be a sequence of strings, it gets mad at me if I add .createWithDefault(Nil) to the config

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.)

@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.

@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 #22192 (comment)? 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

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)

Ah, whoops, misunderstood the value of conf.get. That works just fine

vanzin · 2018-08-29T18:44:06Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+  Thread.currentThread().setContextClassLoader(replClassLoader)
+  conf.get(EXECUTOR_PLUGINS).foreach { classes =>
+    Utils.loadExtensions(classOf[ExecutorPlugin], classes, conf)
+      .foreach { _.init() }


nit: .foreach(_.init()) or .foreach { p => p.init() }

vanzin · 2018-08-29T18:44:35Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+  private JavaSparkContext sc;
+
+  private String EXECUTOR_PLUGIN_CONF_NAME = "spark.executor.plugins";
+  private String testPluginName = "org.apache.spark.ExecutorPluginSuite$TestExecutorPlugin";


TestExecutorPlugin.class.getName()?

vanzin · 2018-08-29T18:45:55Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+
+    assertEquals(2, numSuccessfulPlugins);
+
+    if (sc != null) {


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...

mridulm

I love this idea, thanks for working on it @squito, @NiharS !

core/src/main/java/org/apache/spark/ExecutorPlugin.java

mridulm · 2018-08-29T20:16:24Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+  // 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 =>


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.

mridulm · 2018-08-29T20:24:32Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+    // Wait a moment since plugins run on separate threads
+    Thread.sleep(500);
+
+    assertEquals(1, numSuccessfulPlugins);


After introducing an explicit start(), we can replace the sleep with a timed wait on a condition.

NiharS · 2018-08-30T22:11:42Z

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 starts the plugins so really plugin writers wouldn't have to even worry about making their plugins async. Does anyone have strong opinions?

… optional

SparkQA · 2018-08-31T05:02:17Z

Test build #95503 has finished for PR 22192 at commit 2907c6b.

This patch fails PySpark unit tests.
This patch merges cleanly.
This patch adds no public classes.

tgravescs · 2018-08-31T13:29:45Z

core/src/main/java/org/apache/spark/ExecutorPlugin.java

+ */
+@DeveloperApi
+public interface ExecutorPlugin {
+  default void init() {}


javadoc for init

tgravescs · 2018-08-31T13:33:05Z

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


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

tgravescs · 2018-08-31T13:35:38Z

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)


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.

tgravescs · 2018-08-31T13:37:28Z

ok to test

SparkQA · 2018-08-31T17:44:55Z

Test build #95557 has finished for PR 22192 at commit a968b0a.

This patch fails Scala style tests.
This patch merges cleanly.
This patch adds no public classes.

… msg

SparkQA · 2018-09-10T07:05:01Z

Test build #95862 has finished for PR 22192 at commit 447c5e5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

NiharS · 2018-09-10T17:25:41Z

Logs suddenly cut off again without any exceptions, don't think this one is a code error as well. Could someone retest this please?

vanzin · 2018-09-10T17:32:28Z

retest this please

SparkQA · 2018-09-10T21:41:17Z

Test build #95887 has finished for PR 22192 at commit 447c5e5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

squito · 2018-09-10T21:58:49Z

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.

NiharS · 2018-09-10T23:41:27Z

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

SparkQA · 2018-09-11T01:46:00Z

Test build #4333 has finished for PR 22192 at commit 447c5e5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

bersprockets · 2018-09-11T02:43:53Z

retest this please.

It's that old "java.lang.reflect.InvocationTargetException: null" error we've seen many times.

felixcheung · 2018-09-11T04:08:26Z

Jenkins, retest this please

SparkQA · 2018-09-11T07:05:02Z

Test build #95915 has finished for PR 22192 at commit 447c5e5.

This patch fails due to an unknown error code, -9.
This patch merges cleanly.
This patch adds no public classes.

bersprockets · 2018-09-11T14:57:48Z

retest this please

SparkQA · 2018-09-11T20:08:22Z

Test build #95947 has finished for PR 22192 at commit 447c5e5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

1. Use ExecutorPlugin interface from apache/spark#22192 for SPARK-24918. There is no support for taskStart, onTaskFailure and onTaskCompletion, so MemoryMonitorExecutorExtension cannot support the polling for thread dumps. 2. Add support for procfs memory metrics.

NiharS · 2018-09-17T23:13:22Z

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!

vanzin · 2018-09-19T01:02:36Z

LGTM.

vanzin · 2018-09-19T01:02:41Z

retest this please

SparkQA · 2018-09-19T04:53:17Z

Test build #96212 has finished for PR 22192 at commit 447c5e5.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-09-19T05:44:31Z

Test build #96211 has finished for PR 22192 at commit 447c5e5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

jiangxb1987

LGTM too only some nits.

jiangxb1987 · 2018-09-19T15:52:37Z

core/src/main/scala/org/apache/spark/executor/Executor.scala

+      val pluginList: Seq[ExecutorPlugin] =
+        Utils.withContextClassLoader(replClassLoader) {
+          val plugins = Utils.loadExtensions(classOf[ExecutorPlugin], pluginNames, conf)
+          plugins.foreach(_.init())


nit: Might be good to log whether each plugin.init() succeeded.

jiangxb1987 · 2018-09-19T16:02:29Z

core/src/test/java/org/apache/spark/ExecutorPluginSuite.java

+  }
+
+  @Test
+  public void testAddMultiplePlugins() throws InterruptedException {


super nit: shall we test whether we can load multiple different plugins?

SparkQA · 2018-09-19T23:05:53Z

Test build #96276 has finished for PR 22192 at commit f853fe5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

vanzin · 2018-09-20T18:51:37Z

merging to master / 2.4.

## What changes were proposed in this pull request? 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. 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. Closes #22192 from NiharS/executorPlugin. Lead-authored-by: Nihar Sheth <niharrsheth@gmail.com> Co-authored-by: NiharS <niharrsheth@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com> (cherry picked from commit 2f51e72) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com>

cloud-fan · 2018-09-26T03:14:26Z

why do we merge a new API to branch-2.4, when RC1 is already out?

vanzin · 2018-09-26T16:15:09Z

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.

1. Use ExecutorPlugin interface from apache/spark#22192 for SPARK-24918. There is no support for taskStart, onTaskFailure and onTaskCompletion, so MemoryMonitorExecutorExtension cannot support the polling for thread dumps. 2. Add support for procfs memory metrics.

HyukjinKwon · 2019-11-07T01:56:52Z

core/src/main/java/org/apache/spark/ExecutorPlugin.java

+ * 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


Seems like DeveloperApi already implies though.

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. Executor plugins now run on one separate thread, so the executor does not wait on them. Added testing. Added test cases that test using a sample plugin. Closes apache#22192 from NiharS/executorPlugin. Lead-authored-by: Nihar Sheth <niharrsheth@gmail.com> Co-authored-by: NiharS <niharrsheth@gmail.com> Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com> (cherry picked from commit 2f51e72) Signed-off-by: Marcelo Vanzin <vanzin@cloudera.com> (cherry picked from commit 43c62e7)

[SPARK-24918] Executor Plugin API

44454dd

This commit adds testing and moves the plugin initialization to a separate thread.

squito mentioned this pull request Aug 23, 2018

[SPARK-24918][Core] Executor Plugin api #21923

Closed

NiharS changed the title ~~[SPARK-24918] Executor Plugin API~~ [SPARK-24918][Core] Executor Plugin API Aug 23, 2018

fixed java style issue and ran java linter

7c86fc5

vanzin reviewed Aug 24, 2018

View reviewed changes

cleaned up tests, doc fixes, removed separate testing class, put stat…

44aa634

…ic class in main testing file

fixed java style issue

284beaf

vanzin reviewed Aug 29, 2018

View reviewed changes

mridulm reviewed Aug 29, 2018

View reviewed changes

added finally clauses to all tests, changed conf to be nil instead of…

2907c6b

… optional

tgravescs reviewed Aug 31, 2018

View reviewed changes

added stop, logging before and after plugins, testing for stop

a968b0a

changed util function to classloader only, fixed test style and error…

447c5e5

… msg

jiangxb1987 reviewed Sep 19, 2018

View reviewed changes

add debug msg on successful plugin load, multiple plugins in test

f853fe5

asfgit closed this in 2f51e72 Sep 20, 2018

HyukjinKwon reviewed Nov 7, 2019

View reviewed changes


		import org.apache.spark.api.java.JavaSparkContext;

		import org.junit.Assert;

[SPARK-24918][Core] Executor Plugin API #22192

[SPARK-24918][Core] Executor Plugin API #22192

Conversation

NiharS commented Aug 22, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

vanzin commented Aug 22, 2018

vanzin commented Aug 22, 2018

felixcheung commented Aug 23, 2018

SparkQA commented Aug 23, 2018

SparkQA commented Aug 23, 2018

NiharS commented Aug 23, 2018

NiharS commented Aug 23, 2018 • edited Loading

vanzin commented Aug 23, 2018

SparkQA commented Aug 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiharS Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SparkQA commented Aug 28, 2018

SparkQA commented Aug 29, 2018

NiharS commented Aug 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mridulm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NiharS commented Aug 30, 2018

SparkQA commented Aug 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgravescs commented Aug 31, 2018

SparkQA commented Aug 31, 2018

SparkQA commented Sep 10, 2018

NiharS commented Sep 10, 2018

vanzin commented Sep 10, 2018

SparkQA commented Sep 10, 2018

squito commented Sep 10, 2018

NiharS commented Sep 10, 2018

SparkQA commented Sep 11, 2018

bersprockets commented Sep 11, 2018

felixcheung commented Sep 11, 2018

SparkQA commented Sep 11, 2018

bersprockets commented Sep 11, 2018

SparkQA commented Sep 11, 2018

NiharS commented Sep 17, 2018

vanzin commented Sep 19, 2018

vanzin commented Sep 19, 2018

SparkQA commented Sep 19, 2018

NiharS commented Aug 22, 2018 •

edited

Loading

NiharS commented Aug 23, 2018 •

edited

Loading

NiharS Aug 27, 2018 •

edited

Loading