-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
SPARK-3874, Provide stable TaskContext API #2782
Conversation
Can one of the admins verify this patch? |
QA tests have started for PR 2782 at commit
|
QA tests have finished for PR 2782 at commit
|
Test FAILed. |
Jenkins, retest this please. |
QA tests have started for PR 2782 at commit
|
QA tests have finished for PR 2782 at commit
|
Test FAILed. |
QA tests have started for PR 2782 at commit
|
QA tests have finished for PR 2782 at commit
|
Test FAILed. |
Is there any easy way to reduce the visibility of If we do that, you could use some tricks to cut down on the amount of boilerplate getter code code in TaskContext. In #2696, for example, I use this pattern: Java interface / abstract base class: public interface SparkJobInfo {
int jobId();
int[] stageIds();
String status();
} Scala implementation: private class SparkJobInfoImpl (
val jobId: Int,
val stageIds: Array[Int],
val status: String)
extends SparkJobInfo |
@@ -37,7 +37,7 @@ | |||
* Contextual information about a task which can be read or mutated during execution. | |||
*/ | |||
@DeveloperApi | |||
public class TaskContext implements Serializable { | |||
public abstract class TaskContext implements Serializable { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer this class not have a constructor (the whole idea here is that we don't want user to instantiate it). Basically we want to move all of the logic here to TaskContextImpl
and only define the interface here in terms of abstract methods.
Hey @ScrapCodes - this is a good start, but I'd prefer to move all of the functionality into TaskContextImpl. Basically, we want |
By the way - if you look at the original design I put in the JIRA, this is what it does. Thinks like |
Hey Patrick, You are right about that. We can make TaskContext an interface if we only allow TaskContextHelper.get() instead of TaskContext.get(). And then maybe I can rename TaskContextHelper to TaskContextGetter ? (Not sure !) |
@ScrapCodes We cant make it a proper interface because of binary compatibility (this is a pretty widely used API so I'd prefer not to break it). |
public boolean isCompleted() { | ||
return completed; | ||
} | ||
public abstract boolean isCompleted(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//cc @rxin - should this be "isComplete" rather than "isCompleted"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually this looks good - we use isCompleted elsewhere already in user-facing code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that isCompleted means it has been completed, whereas isComplete means it is "whole".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think isCompleted is right, since we'd use isFailed instead of isFailure or isFail.
Jenkins, test this please |
QA tests have started for PR 2782 at commit
|
QA tests have finished for PR 2782 at commit
|
Test FAILed. |
No description provided.