SAMZA-1246: ApplicatonRunner.stats() should include exception in case of failure #154
Conversation
|
|
||
| package org.apache.samza.runtime; | ||
|
|
||
| public class ApplicationStatus { |
There was a problem hiding this comment.
So after this PR and @navina change, we're going to have
JobStatus
ApplicationStatus
ContainerStatus
... others?
It'd be nice to have a clear purpose for each of these.
For example, I don't see why ApplicationStatus needs to handle exceptions, but JobStatus doesn't. If anything, I'd think the only difference between the two is that ApplicationStatus spans multiple jobs, so it might have a status to indicate PartialFinish for the case that a subset of the jobs are not running. Otherwise, I don't see why both are needed.
There was a problem hiding this comment.
Makes sense. I reverted back the JobStatus and keep only one ApplicationStatus.
| @@ -106,11 +106,11 @@ public ApplicationStatus status(StreamApplication app) { | |||
| } | |||
|
|
|||
| if (unsuccessfulFinish) { | |||
| return ApplicationStatus.UnsuccessfulFinish; | |||
| return ApplicationStatus.unsuccessfulFinish(null); // no exception for remote execution. | |||
There was a problem hiding this comment.
Feels like this is a gap. Ideally, we should report an exception for the remote case too. Maybe, for example YarnJob should have the smarts to determine why an application failed using the Diagnostic Info
There was a problem hiding this comment.
Good suggestion! IN this case, I created a SamzaException with the diagonostic info. Please take a look. Thanks.
|
btw, I also changed the status computation for RemoteApplicationRunner to be consistent with LocalApplicationRunner. So now it follows the following logic:
|
| @@ -247,7 +247,6 @@ StreamProcessor createStreamProcessor( | |||
| latch.await(); | |||
|
|
|||
| if (throwable.get() != null) { | |||
| appStatus = ApplicationStatus.UnsuccessfulFinish; | |||
| throw throwable.get(); | |||
There was a problem hiding this comment.
I assume appStatus is no longer set here because the next line throws an exception. But on line 136, appStatus is set under the same circumstances. Why is that? Shouldn't run() and waitForFinish() have the same semantics for exception and status handling?
There was a problem hiding this comment.
oh, I removed this line is because it's redundant. As you see, the exception thrown here will be caught in line 136 and the status will be set again. The code is pretty messy right now. I am waiting for @navina 's change to finally clean these up in patch (#135). I will also add the waitForFinish() in that patch.
Current when ApplicationRunner.stats() only returns the enum representing the status. It also need to include the exception if the status is failure.