-
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-10620] [SPARK-13054] Minor addendum to #10835 #10958
[SPARK-10620] [SPARK-13054] Minor addendum to #10835 #10958
Conversation
…-accums-followups Conflicts: core/src/main/scala/org/apache/spark/Accumulable.scala
7e953f2
to
5a24601
Compare
5a24601
to
822899d
Compare
Test build #50234 has finished for PR 10958 at commit
|
With this change, we ALWAYS post task end events in canceled stages for both tasks that succeeded and tasks that failed.
Test build #50235 has finished for PR 10958 at commit
|
Test build #50238 has started for PR 10958 at commit |
retest this please |
Test build #50244 has finished for PR 10958 at commit
|
// introducing additional complexity in the scheduler to wait for all the task end events | ||
// before posting the stage completed event. | ||
listenerBus.post(SparkListenerTaskEnd( | ||
stageId, task.stageAttemptId, taskType, event.reason, event.taskInfo, taskMetrics)) |
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.
(comment for linking)
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.
This should probably have a test like #10951. We should combine our efforts on this. This might make more sense to have in a separate PR from the other style things but doesn't matter to much.
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.
ok, I separated it
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.
are you going to file a separate pr or should I just update mine ?
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.
you can just update yours
@@ -60,19 +60,20 @@ import org.apache.spark.storage.{BlockId, BlockStatus} | |||
* @tparam T result type | |||
*/ | |||
class Accumulator[T] private[spark] ( | |||
@transient private[spark] val initialValue: T, | |||
// SI-8813: This must explicitly be a private val, or else scala 2.11 doesn't compile | |||
@transient private val initialValue: T, |
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.
This change was already added in another patch, causing a merge conflict here.
LGTM. Sorry for the delay in reviewing. |
This reverts commit ba5f555.
…-accums-followups Conflicts: core/src/test/scala/org/apache/spark/InternalAccumulatorSuite.scala
Test build #50943 has finished for PR 10958 at commit
|
I'm going to merge this into master. Thanks! |
Additional changes to #10835, mainly related to style and visibility. This patch also adds back a few deprecated methods for backward compatibility.