You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by victor-wong <gi...@git.apache.org> on 2017/11/27 11:01:12 UTC
[GitHub] spark pull request #19824: Revert "[SPARK-18905][STREAMING] Fix the issue of...
GitHub user victor-wong opened a pull request:
https://github.com/apache/spark/pull/19824
Revert "[SPARK-18905][STREAMING] Fix the issue of removing a failed jobset from JobScheduler.jobSets"
## What changes were proposed in this pull request?
The code changes in PR(https://github.com/apache/spark/pull/16542) make me very confusing:
https://github.com/apache/spark/blob/5a02e3a2ac8a25d92d98d3b3b0d1173dddb9cc91/streaming/src/main/scala/org/apache/spark/streaming/scheduler/JobScheduler.scala#L203
`
private def handleJobCompletion(job: Job, completedTime: Long) {
val jobSet = jobSets.get(job.time)
jobSet.handleJobCompletion(job)
job.setEndTime(completedTime)
listenerBus.post(StreamingListenerOutputOperationCompleted(job.toOutputOperationInfo))
logInfo("Finished job " + job.id + " from job set of time " + jobSet.time)
if (jobSet.hasCompleted) {
listenerBus.post(StreamingListenerBatchCompleted(jobSet.toBatchInfo))
}
job.result match {
case Failure(e) =>
reportError("Error running job " + job, e)
case _ =>
if (jobSet.hasCompleted) {
jobSets.remove(jobSet.time)
jobGenerator.onBatchCompletion(jobSet.time)
logInfo("Total delay: %.3f s for time %s (execution: %.3f s)".format(
jobSet.totalDelay / 1000.0, jobSet.time.toString,
jobSet.processingDelay / 1000.0
))
}
}
}
`
If a Job failed and the JobSet containing it has completed, listenerBus will post a StreamingListenerBatchCompleted, while jobGenerator will not invoke onBatchCompletion. So the batch is completed or not ?
The key point is if a Job in a Batch failed, whether or not we consider the Batch as completed.
I think if someone register a listener on StreamingListenerBatchCompleted, he just wants to get notified only when the batch finishes with no error. So if a Job is failed, we should not remove it from its JobSet, thus the JobSet has not completed.
## How was this patch tested?
existing tests
Please review http://spark.apache.org/contributing.html before opening a pull request.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/victor-wong/spark revert-job-completion
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19824.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #19824
----
commit aafe7b62f80ff1e86f6c528711d24e7de54536c5
Author: wangjiasheng <wa...@xiaomi.com>
Date: 2017-11-27T08:28:48Z
Revert "[SPARK-18905][STREAMING] Fix the issue of removing a failed jobset from JobScheduler.jobSets"
This reverts commit f8db8945f25cb884278ff8841bac5f6f28f0dec6.
commit e4e57cca9b0d21db8ad6292f8fcbde2dd316d7b7
Author: wangjiasheng <wa...@xiaomi.com>
Date: 2017-11-27T08:31:24Z
[SPARK][STREAMING] Invoke onBatchCompletion() only when all jobs in the JobSet are Success
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by victor-wong <gi...@git.apache.org>.
Github user victor-wong commented on the issue:
https://github.com/apache/spark/pull/19824
@CodingCat
Thank you:)
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the issue:
https://github.com/apache/spark/pull/19824
#16542 has guaranteed that the failed batch can be re-executed, and I didn’t check if reverting the change in #16542 plus your new change can guarantee the same thing...
Suppose it also guarantees, the remaining discussion becomes “what does complete mean in English?” which is not interesting to me to discuss
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: Revert "[SPARK-18905][STREAMING] Fix the issue of removi...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the issue:
https://github.com/apache/spark/pull/19824
did I miss anything? @victor-wong , you are describing what https://github.com/apache/spark/pull/16542 does in your description, but you are reverting it in your changes?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: Revert "[SPARK-18905][STREAMING] Fix the issue of removi...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19824
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by victor-wong <gi...@git.apache.org>.
Github user victor-wong commented on the issue:
https://github.com/apache/spark/pull/19824
@viirya Sorry for the misleading title, I have changed it now.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19824: [SPARK][STREAMING] Invoke onBatchCompletion() onl...
Posted by victor-wong <gi...@git.apache.org>.
Github user victor-wong closed the pull request at:
https://github.com/apache/spark/pull/19824
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by victor-wong <gi...@git.apache.org>.
Github user victor-wong commented on the issue:
https://github.com/apache/spark/pull/19824
@CodingCat Yes, this PR wants to solve the same issue in https://github.com/apache/spark/pull/16542, but I think this is a better way to solve it.
If a Job failed, I think we should not remove it from its JobSet, so `jobSet.hasCompleted` will return false. As a result, we will not receive a StreamingListenerBatchCompleted.
What I want to say is that if a Job is failed, we should consider the Batch as not completed.
I am not confident about my English, if I am not describing it clear, please let me know.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the issue:
https://github.com/apache/spark/pull/19824
One thing to note is that mute an event is a behavior change, if a user has introduced some customized listener to capture all completed batches and also extract failed job info, he/she will see a broken scenario with the change in this PR
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the issue:
https://github.com/apache/spark/pull/19824
`What I want to say is that if a Job is failed, we should consider the Batch as not completed.` isn't #16542 doing the same thing?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by CodingCat <gi...@git.apache.org>.
Github user CodingCat commented on the issue:
https://github.com/apache/spark/pull/19824
if you just worry about
> As I was using the StreamingListenerBatchCompleted to do some metadata checkpointing stuff, which should be done only when the batch succeeded.
If this is your concern, you should handle whether complete is with a failure in your own listener, check: https://github.com/Azure/azure-event-hubs-spark/blob/fecc34de8a238049806d033d8a85d888cad75901/core/src/main/scala/org/apache/spark/streaming/eventhubs/checkpoint/ProgressTrackingListener.scala#L38
IMHO, `Complete` includes Failure&Success cases is more intuitive to the (scala) user since we are used to .OnComplete in Scala Future, etc.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by victor-wong <gi...@git.apache.org>.
Github user victor-wong commented on the issue:
https://github.com/apache/spark/pull/19824
@CodingCat please checkout the difference between the two PR.
`
if (jobSet.hasCompleted) {
- jobSets.remove(jobSet.time)
- jobGenerator.onBatchCompletion(jobSet.time)
- logInfo("Total delay: %.3f s for time %s (execution: %.3f s)".format(
- jobSet.totalDelay / 1000.0, jobSet.time.toString,
- jobSet.processingDelay / 1000.0
- ))
listenerBus.post(StreamingListenerBatchCompleted(jobSet.toBatchInfo))
}
`
As shown above, in In https://github.com/apache/spark/pull/16542, if a Job failed, listenerBus still post a StreamingListenerBatchCompleted, which I believe to be incorrect, because the Batch is not completed (a Job of it has failed).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: Revert "[SPARK-18905][STREAMING] Fix the issue of removi...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19824
cc @zsxwing
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: [SPARK][STREAMING] Invoke onBatchCompletion() only when ...
Posted by victor-wong <gi...@git.apache.org>.
Github user victor-wong commented on the issue:
https://github.com/apache/spark/pull/19824
@CodingCat
> One thing to note is that mute an event is a behavior change
I agree with that, so we should be careful about changing the current behavior. I will close the PR later.
> the remaining discussion becomes “what does complete mean in English?” which is not interesting to me to discuss
Maybe the remaining discussion should be how to let the user know that he will get a StreamingListenerBatchCompleted event even if the batch failed.
What about adding some comments:
`
/** Called when processing of a batch of jobs has completed **(event if the batch failed)**. */
def onBatchCompleted(batchCompleted: StreamingListenerBatchCompleted) { }
`
As I was using the StreamingListenerBatchCompleted to do some metadata checkpointing stuff, which should be done only when the batch succeeded.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: Revert "[SPARK-18905][STREAMING] Fix the issue of removi...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/19824
cc @CodingCat, WDYT about this revert?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19824: Revert "[SPARK-18905][STREAMING] Fix the issue of removi...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/19824
Btw, I think this is not only revert the commit, seems it also proposes its fix. It is better to modify the title.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org