[SPARK-21253][Core]Fix a bug that StreamCallback may not be notified if network errors happen #18472
Conversation
|
cc @vanzin |
|
LGTM, cc @jiangxb1987 |
|
Nice catch! |
| } | ||
| for (Tuple2<String, StreamCallback> entry : streamCallbacks) { | ||
| try { | ||
| entry._2().onFailure(entry._1(), cause); |
There was a problem hiding this comment.
Seems the streamId is never used in DownloadCallback.onFailure() ?
| } | ||
| for (Tuple2<String, StreamCallback> entry : streamCallbacks) { | ||
| try { | ||
| entry._2().onFailure(entry._1(), cause); |
There was a problem hiding this comment.
Actually StreamCallback has a 1-1 mapping to the steam, so ideally we don't need to pass streamId when calling onFailure or onSuccess, the place that creates the StreamCallback already knows which stream this callback is resposible to. I also checked all implemetations of StreamCallback, all of them ignore the streamId parameter. Shall we update the StreamCallback interface?
There was a problem hiding this comment.
I think keeping streamId doesn't hurt. May be helpful to debug, and also it's consistent with ChunkReceivedCallback.
|
Test build #78939 has finished for PR 18472 at commit
|
|
thanks, merging to master/2.2! we can do the stream id clean up later. |
… if network errors happen ## What changes were proposed in this pull request? If a network error happens before processing StreamResponse/StreamFailure events, StreamCallback.onFailure won't be called. This PR fixes `failOutstandingRequests` to also notify outstanding StreamCallbacks. ## How was this patch tested? The new unit tests. Author: Shixiong Zhu <[email protected]> Closes #18472 from zsxwing/fix-stream-2. (cherry picked from commit 4996c53) Signed-off-by: Wenchen Fan <[email protected]>
## What changes were proposed in this pull request? A follow up PR to fix Scala 2.10 build for #18472 ## How was this patch tested? Jenkins Author: Shixiong Zhu <[email protected]> Closes #18478 from zsxwing/SPARK-21253-2. (cherry picked from commit cfc696f) Signed-off-by: Shixiong Zhu <[email protected]>
## What changes were proposed in this pull request? A follow up PR to fix Scala 2.10 build for #18472 ## How was this patch tested? Jenkins Author: Shixiong Zhu <[email protected]> Closes #18478 from zsxwing/SPARK-21253-2.
5 participants
What changes were proposed in this pull request?
If a network error happens before processing StreamResponse/StreamFailure events, StreamCallback.onFailure won't be called.
This PR fixes
failOutstandingRequeststo also notify outstanding StreamCallbacks.How was this patch tested?
The new unit tests.