-
Notifications
You must be signed in to change notification settings - Fork 28.1k
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 #2803
Conversation
Jenkins, test this please. With these changes this LGTM. |
QA tests have started for PR 2803 at commit
|
QA tests have started for PR 2803 at commit
|
QA tests have finished for PR 2803 at commit
|
Test FAILed. |
QA tests have finished for PR 2803 at commit
|
Test PASSed. |
@@ -40,6 +40,7 @@ object MimaBuild { | |||
ProblemFilters.exclude[IncompatibleResultTypeProblem](fullName), | |||
ProblemFilters.exclude[IncompatibleMethTypeProblem](fullName), | |||
ProblemFilters.exclude[IncompatibleFieldTypeProblem](fullName) | |||
// ProblemFilters.exclude[AbstractClassProblem](fullName) |
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.
Could we just remove this instead of commenting it out?
LGTM. |
} | ||
public abstract boolean isInterrupted(); | ||
|
||
@Deprecated |
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.
super nit: add annotation after javadoc.
Thanks @JoshRosen and @vanzin - I've pushed a patch to address the style issues. |
Test FAILed. |
Jenkins, retest this please. |
Test FAILed. |
QA tests have started for PR 2803 at commit
|
"org.apache.spark.scheduler.MapStatus"), | ||
// TaskContext was promoted to Abstract class | ||
ProblemFilters.exclude[AbstractClassProblem]( | ||
"org.apache.spark.TaskContext") |
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 felt we could exclude this generally, it did not made sense to me to report these errors for classes marked developerApi or experimental. Thoughts ?
QA tests have finished for PR 2803 at commit
|
Jenkins, test this please. |
QA tests have started for PR 2803 at commit
|
QA tests have finished for PR 2803 at commit
|
Test FAILed. |
Jenkins, test this please. @liancheng - I am getting a SQL failure here that seems like it might be legitimate but I'm having trouble interpreting why it could possibly be caused by this patch - any idea? |
Jenkins, test this please. |
QA tests have started for PR 2803 at commit
|
QA tests have finished for PR 2803 at commit
|
Test FAILed. |
Jenkins, test this please. |
QA tests have started for PR 2803 at commit
|
QA tests have finished for PR 2803 at commit
|
Test PASSed. |
Great @liancheng that seems to have done it. |
@pwendell I'm not pretty sure about that... This PR fixes the race condition, but the first Jenkins build of this PR also suffered the |
This is a small number of clean-up changes on top of #2782. Closes #2782.