Skip to content

Conversation

@NicoK
Copy link
Contributor

@NicoK NicoK commented Feb 10, 2017

This adds a JVM-terminating handler that logs errors from uncaught exceptions
and terminates the process so that critical exceptions are not accidentally
lost and leave the system running in an inconsistent state.

It borrows and re-uses code from @StephanEwen from this PR:
#3290

@uce
Copy link
Contributor

uce commented Feb 10, 2017

Should we instead of copying the uncaught exception handler code move it out to an outer class? You would have to base this PR on #3290, move the class out and only use it here.

@NicoK
Copy link
Contributor Author

NicoK commented Feb 10, 2017

would be a different LOG handler though - does it make sense to have two or is it enough to have a single one in an outer class?

@uce
Copy link
Contributor

uce commented Feb 10, 2017

I think it's logged as FatalExitExceptionHandler in both places right now as well (Stephan's PR and your PR). So nothing would change if we move it out, or am I overlooking something?

@NicoK
Copy link
Contributor Author

NicoK commented Feb 10, 2017

wouldn't it be NettyServer$FatalExitExceptionHandler vs. ExecutorThreadFactory$FatalExitExceptionHandler?

I'm just asking - don't know whether it is a valid use case to have the two or if it's good enough to have one only...

@uce
Copy link
Contributor

uce commented Feb 10, 2017

I thought that this is only going to be one, sorry. Imo only one is good. The thread name is logged in the message.

@StephanEwen
Copy link
Contributor

Looking at this and at my previous pull request, I am wondering if we should actually define and collect exit codes somewhere globally, like in flink-core:org.apache.flink.configuration.ExitCodes.

@NicoK
Copy link
Contributor Author

NicoK commented Feb 10, 2017

I was actually looking through the code to find something like this but it seems that every class does this locally for now. Global exit codes make sense though - also for documentation

@NicoK
Copy link
Contributor Author

NicoK commented Feb 10, 2017

@uce I'll extract the inner class and use it here as well as soon as the final #3290 is merged

Nico Kruber added 2 commits February 10, 2017 18:31
Make it a top-level class so that it can be re-used.
This sets a JVM-terminating handler that logs errors from uncaught exceptions
and terminates the process so that critical exceptions are not accidentally
lost and leave the system running in an inconsistent state.

extend
@StephanEwen
Copy link
Contributor

#3290 is in

@StephanEwen
Copy link
Contributor

@NicoK I think you can update this now...

@NicoK
Copy link
Contributor Author

NicoK commented Feb 14, 2017

@StephanEwen already did when #3290 got in

@uce
Copy link
Contributor

uce commented Feb 14, 2017

This looks good to me now. Nico moved the FatalExitExceptionHandler out of ExecutorThreadFactory. I'm going to merge this later today if @StephanEwen has no objections.

@StephanEwen
Copy link
Contributor

+1 to merge this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants