SAMZA-1220 : Add thread name to SamzaContainer shutdown hook and prevent shutdown deadlock #139
Conversation
navina
commented
Apr 25, 2017
navina
commented
- SamzaContainerExceptionHandler is written in Java and used by LocalContainerRunner.java
|
@prateekm Can you please take a look at the changes? Please comment if this was not what you had in mind. Thanks! |
|
|
||
| /** | ||
| * An UncaughtExceptionHandler for SamzaContainer that simply shuts down when any thread throws | ||
| * an uncaught exception. |
There was a problem hiding this comment.
Minor:
Documentation could use slight re-wording? This does not invoke System.exit itself right? IIUC, It simply runs the provided Runnable?
Or, we could get rid off the runnable, and make the code and documentation consistent. (It's a matter of minor convenience anyways)
There was a problem hiding this comment.
I think this is a result of copying the documentation over from the scala class. I will fix the documentation.
There was a problem hiding this comment.
+1 if you want to remove the Runnable parameter.
| import org.apache.samza.SamzaException | ||
| import org.junit.After | ||
| import org.apache.samza.SamzaException; | ||
| import org.junit.Test; |
There was a problem hiding this comment.
Minor: organize these imports.
There was a problem hiding this comment.
That is how IntelliJ organizes imports. Were you referring some specific ordering?
There was a problem hiding this comment.
Makes sense, I thought the junit imports could have been potentially grouped together. I think I may have missed the static clause in the assertTrue.
| try { | ||
| runLoopThread.join(shutdownMs) | ||
| } catch { | ||
| case e: Exception => // Ignore to avoid deadlock with uncaughtExceptioHandler. See SAMZA-1220 |
There was a problem hiding this comment.
IMHO it makes sense to catch, log and ignore all Throwable here.
- OOM errors happening in shutdown hook are usually in-actionable.
- We will bound the wait time by
shutdownMsanyways. - Other throwables happening in the
shutdownHookwill end up calling theUncaughtExceptionHandlerresulting in a potential deadlock. Catching allThrowables will prevent this.
There was a problem hiding this comment.
+1 for catching Throwable. Maybe also move the logging to try instead of finally (in case it throws).
prateekm
left a comment
prateekm
left a comment
There was a problem hiding this comment.
Looks good, thanks. Some minor comments.
| } | ||
| } catch { | ||
| case e: IllegalStateException => { | ||
| // When samza is shutdown by external command, IllegalStateException will be thrown. |
There was a problem hiding this comment.
Maybe rephrase to:
"Thrown when then JVM is already shutting down, so safe to ignore."?
|
|
||
| /** | ||
| * An UncaughtExceptionHandler for SamzaContainer that simply shuts down when any thread throws | ||
| * an uncaught exception. |
There was a problem hiding this comment.
+1 if you want to remove the Runnable parameter.
| try { | ||
| runLoopThread.join(shutdownMs) | ||
| } catch { | ||
| case e: Exception => // Ignore to avoid deadlock with uncaughtExceptioHandler. See SAMZA-1220 |
There was a problem hiding this comment.
+1 for catching Throwable. Maybe also move the logging to try instead of finally (in case it throws).
3 participants
