SPARK-2636: Expose job ID in JobWaiter API #2176

lirui-apache · 2014-08-28T06:05:00Z

This PR adds the async actions to the Java API. User can call these async actions to get the FutureAction and use JobWaiter (for SimpleFutureAction) to retrieve job Id.

SparkQA · 2014-08-28T06:11:04Z

QA tests have started for PR 2176 at commit fbf5744.

This patch merges cleanly.

rxin · 2014-08-28T06:14:47Z

@lirui-intel - JobWaiter is an internal API that's never designed to be public. I would not expose it simply because you need the job id. There are lots of ways to get the job id ... e.g. you can add an interface to the future to get the job id.

Also this doesn't really make sense. Even with your change, JobWaiter is still private[spark], and only SimpleFutureAction contains a public field to it ...

lirui-apache · 2014-08-28T06:47:03Z

Hi @rxin, thanks for the review! I can add interface to SimpleFutureAction to get the job id if we shouldn't expose JobWaiter to users.
Hive on spark currently only uses foreach to submit the job, we can change it to foreachAsync which just returns a SimpleFutureAction.

I agree this doesn't seem to be a perfect way to solve the problem and we still have ComplexFutureAction to worry about. So please let me know if you have a better idea in mind or if you think we shouldn't expose the job ID at all.

rxin · 2014-08-28T06:49:20Z

If foreachAsync is the only one you need right now, why don't you just add foreachAsync (and remove the rest), and add jobId to SimpleFutureAction?

lirui-apache · 2014-08-28T06:57:54Z

I thought these async actions are missing in the java API so I added all of them from AsyncRDDActions. But sure, let me just add foreachAsync.

SparkQA · 2014-08-28T06:59:23Z

QA tests have finished for PR 2176 at commit fbf5744.

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

rxin · 2014-08-28T07:01:41Z

Yea let's not add all of them since they are highly experimental. I'm not even sure if those are the APIs we want to commit to in the long run.

lirui-apache · 2014-08-28T07:31:48Z

@rxin I've updated the patch.
Yes I see these APIs are experimental. We can make hive use it as a workaround and change it when we have a better solution.

rxin · 2014-08-29T05:56:28Z

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

+  /**
+   * Get the corresponding job Id for this action
+   */
+  def jobId(): Int = {


maybe without the parenthesis, and lower case id?

/** Get the corresponding job id for this action. */ def jobId = jobWaiter.jobId

rxin · 2014-08-29T05:58:12Z

Jenkins, ok to test.

SparkQA · 2014-08-29T06:04:06Z

QA tests have started for PR 2176 at commit af4f5d9.

This patch merges cleanly.

lirui-apache · 2014-08-29T06:38:56Z

Thanks @rxin . Updated the patch accordingly.

SparkQA · 2014-08-29T06:39:14Z

QA tests have started for PR 2176 at commit 3fa39f7.

This patch merges cleanly.

rxin · 2014-08-29T06:42:06Z

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

+  /**
+   * Get the corresponding job Id for this action
+   */
+  def id: Int = {


hey I meant jobId, not id ...

I was referring to the lowercase id in your javadoc... take a look at my previous comment

Oops, sorry about that.

SparkQA · 2014-08-29T06:51:27Z

QA tests have finished for PR 2176 at commit af4f5d9.

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

SparkQA · 2014-08-29T06:54:07Z

QA tests have started for PR 2176 at commit 0ca320d.

This patch merges cleanly.

SparkQA · 2014-08-29T07:35:50Z

QA tests have finished for PR 2176 at commit 3fa39f7.

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

rxin · 2014-08-29T07:37:51Z

You will need to add mima excludes.

SparkQA · 2014-08-29T07:50:11Z

QA tests have finished for PR 2176 at commit 0ca320d.

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

lirui-apache · 2014-08-29T07:52:32Z

Hi @rxin , could you be more specific as how to do it?

rxin · 2014-08-29T07:53:42Z

Take a look at the failure message here: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19456/consoleFull

And look at file in project/Mima*

lirui-apache · 2014-08-29T09:12:39Z

Thanks @rxin . I updated the patch.

SparkQA · 2014-08-29T09:14:42Z

QA tests have started for PR 2176 at commit e2e01d5.

This patch merges cleanly.

SparkQA · 2014-08-29T10:12:06Z

QA tests have finished for PR 2176 at commit e2e01d5.

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

rxin · 2014-08-29T16:59:16Z

core/src/main/scala/org/apache/spark/api/java/JavaRDDLike.scala

+   * @param f the function to apply to all the elements of the RDD
+   * @return a FutureAction for the action
+   */
+  def foreachAsync(f: VoidFunction[T]): FutureAction[Unit] = {


Can we mark this as experimental in Java?

I don't know if we have a way to mark stuff as experimental, but maybe put this in the javadoc:

* THIS IS AN EXPERIMENTAL API THAT MIGHT CHANGE IN THE FUTURE.

@pwendell thoughts?

In Scala-land the javadocs start like this:

* :: Experimental ::

Perhaps just prepend that to the first line in the javadoc (so the summary contains both the short description and that it's experimental).

@rxin @lirui-intel Make it consistent with the other code where we do this. There is no need to have a big sentence with caps. The ::Experimental:: string along with the @Experimental annotation is what we do. Look in other Java code that has this.

vanzin · 2014-08-29T17:48:10Z

+1 pending the experimental thing. This will be very useful. :-)

Current matching behavior misses JIRA issue keys in titles that don't have brackets, like in [this issue](apache/spark#2176).

rxin · 2014-08-30T05:36:09Z

This patch looks good to me as long as we mark the new java api as experimental.

SparkQA · 2014-08-30T14:34:06Z

QA tests have started for PR 2176 at commit 5536d55.

This patch merges cleanly.

lirui-apache · 2014-08-30T14:37:53Z

Thanks @rxin , @vanzin for the review. I've added experimental mark in the java doc. I see that mima can automatically exclude DeveloperApi and Experimental classes, not sure if the mima exclude is unnecessary now?

SparkQA · 2014-08-30T15:30:40Z

QA tests have finished for PR 2176 at commit 5536d55.

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

pwendell · 2014-08-31T04:19:45Z

I added a comment about the experimental formatting

lirui-apache · 2014-09-01T01:23:56Z

Thanks @pwendell , patch updated.

SparkQA · 2014-09-01T01:29:10Z

QA tests have started for PR 2176 at commit ccaafb7.

This patch merges cleanly.

SparkQA · 2014-09-01T02:30:26Z

QA tests have finished for PR 2176 at commit ccaafb7.

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

rxin · 2014-09-02T06:28:07Z

Thanks. I'm merging this in master.

This PR adds the async actions to the Java API. User can call these async actions to get the FutureAction and use JobWaiter (for SimpleFutureAction) to retrieve job Id. Author: lirui <rui.li@intel.com> Closes apache#2176 from lirui-intel/SPARK-2636 and squashes the following commits: ccaafb7 [lirui] SPARK-2636: fix java doc 5536d55 [lirui] SPARK-2636: mark the async API as experimental e2e01d5 [lirui] SPARK-2636: add mima exclude 0ca320d [lirui] SPARK-2636: fix method name & javadoc 3fa39f7 [lirui] SPARK-2636: refine the patch af4f5d9 [lirui] SPARK-2636: remove unused imports 843276c [lirui] SPARK-2636: only keep foreachAsync in the java API fbf5744 [lirui] SPARK-2636: add more async actions for java api 1b25abc [lirui] SPARK-2636: expose some fields in JobWaiter d09f732 [lirui] SPARK-2636: fix build eb1ee79 [lirui] SPARK-2636: change some parameters in SimpleFutureAction to member field 6e2b87b [lirui] SPARK-2636: add java API for async actions

lirui added 5 commits August 27, 2014 13:02

SPARK-2636: add java API for async actions

6e2b87b

SPARK-2636: change some parameters in SimpleFutureAction to member field

eb1ee79

SPARK-2636: fix build

d09f732

SPARK-2636: expose some fields in JobWaiter

1b25abc

SPARK-2636: add more async actions for java api

fbf5744

lirui added 2 commits August 28, 2014 15:22

SPARK-2636: only keep foreachAsync in the java API

843276c

SPARK-2636: remove unused imports

af4f5d9

lirui-apache changed the title ~~SPARK-2636: no where to get job identifier while submit spark job through spark API~~ SPARK-2636: Expose job ID in JobWaiter API Aug 29, 2014

rxin reviewed Aug 29, 2014
View reviewed changes

SPARK-2636: refine the patch

3fa39f7

rxin reviewed Aug 29, 2014
View reviewed changes

SPARK-2636: fix method name & javadoc

0ca320d

SPARK-2636: add mima exclude

e2e01d5

rxin reviewed Aug 29, 2014
View reviewed changes

nchammas added a commit to nchammas/spark-pr-dashboard that referenced this pull request Aug 29, 2014

Don't require brackets around JIRA issue key

da39438

Current matching behavior misses JIRA issue keys in titles that don't have brackets, like in [this issue](apache/spark#2176).

nchammas mentioned this pull request Aug 29, 2014

Don't require brackets around JIRA issue key databricks/spark-pr-dashboard#9

Closed

SPARK-2636: mark the async API as experimental

5536d55

SPARK-2636: fix java doc

ccaafb7

asfgit closed this in fbf2678 Sep 2, 2014

lirui-apache deleted the SPARK-2636 branch September 3, 2014 06:09

JoshRosen mentioned this pull request Oct 11, 2014

[SPARK-3902] [SPARK-3590] Stabilize AsynRDDActions and add Java API #2760

Closed

SPARK-2636: Expose job ID in JobWaiter API #2176

SPARK-2636: Expose job ID in JobWaiter API #2176

Conversation

lirui-apache commented Aug 28, 2014

SparkQA commented Aug 28, 2014

rxin commented Aug 28, 2014

lirui-apache commented Aug 28, 2014

rxin commented Aug 28, 2014

lirui-apache commented Aug 28, 2014

SparkQA commented Aug 28, 2014

rxin commented Aug 28, 2014

lirui-apache commented Aug 28, 2014

rxin Aug 29, 2014

Choose a reason for hiding this comment

rxin commented Aug 29, 2014

SparkQA commented Aug 29, 2014

lirui-apache commented Aug 29, 2014

SparkQA commented Aug 29, 2014

rxin Aug 29, 2014

Choose a reason for hiding this comment

lirui-apache Aug 29, 2014

Choose a reason for hiding this comment

SparkQA commented Aug 29, 2014

SparkQA commented Aug 29, 2014

SparkQA commented Aug 29, 2014

rxin commented Aug 29, 2014

SparkQA commented Aug 29, 2014

lirui-apache commented Aug 29, 2014

rxin commented Aug 29, 2014

lirui-apache commented Aug 29, 2014

SparkQA commented Aug 29, 2014

SparkQA commented Aug 29, 2014

rxin Aug 29, 2014

Choose a reason for hiding this comment

vanzin Aug 29, 2014

Choose a reason for hiding this comment

pwendell Aug 30, 2014

Choose a reason for hiding this comment

vanzin commented Aug 29, 2014

rxin commented Aug 30, 2014

SparkQA commented Aug 30, 2014

lirui-apache commented Aug 30, 2014

SparkQA commented Aug 30, 2014

pwendell commented Aug 31, 2014

lirui-apache commented Sep 1, 2014

SparkQA commented Sep 1, 2014

SparkQA commented Sep 1, 2014

rxin commented Sep 2, 2014