Call exceptionHandler if Bookie.start fails with exception.#2173
Call exceptionHandler if Bookie.start fails with exception.#2173eolivelli merged 3 commits intoapache:masterfrom
Conversation
When main thread of Bookie/BookieServer has exited with exception while starting Bookie, Bookie process shouldn't be alive because of any non-daemon thread that has already started.
|
@karanmehta93 fyi. I'm not able to add you as reviewer. |
| /* | ||
| * Since Bookie/BookieServer/BookieService is expected to fail to start | ||
| */ | ||
| startFuture.get(); |
There was a problem hiding this comment.
Isn't this line expected to throw an exception?
There was a problem hiding this comment.
No. When exceptionHandler is called, which would trigger shutdownHookThread .
in ComponentShutdownHook.run method, component is closed and then Future is marked complete. Unless, component.close failed with exception this future would complete normally.
There was a problem hiding this comment.
That's was my suspect but the test is not clear.
Can we verify that the handler has been called?
There was a problem hiding this comment.
The comment reads 'it is expected to fail' but the future is not failing, it is not very clear
There was a problem hiding this comment.
Can we verify that the handler has been called?
It is little tricky to test that since the creation and setting of handler and starting of the component are happening in ComponentStarter.startComponent method. Anyhow I came up with other solution where we check if the particular log line saying "exceptionHandler is triggered" is getting logged or not. as part of this scenario.
The comment reads 'it is expected to fail' but the future is not failing, it is not very clear
added more detailed description for what we are doing here.
- added testlogic to test if the exceptionhandler is called - fixing logs and checkstyle violations
|
run bookkeeper-server bookie tests |
|
run bookkeeper-server replication tests |
|
@eolivelli @sijie @jvrao this should be good to go. All checks have passed as well. |
eolivelli
left a comment
eolivelli
left a comment
There was a problem hiding this comment.
LGTM
I will merge if no one else has comments
|
@eolivelli I am taking a look now, will send the review soon. Thanks! |
| } catch (Throwable exc) { | ||
| LOG.error("Got unexpected exception while starting BookieServer", exc); | ||
| if (uncaughtExceptionHandler != null) { | ||
| LOG.error("Calling uncaughtExceptionHandler"); |
There was a problem hiding this comment.
nit: add more details to this log line?
| public static final String NAME = "bookie-server"; | ||
|
|
||
| private final BookieServer server; | ||
| //private UncaughtExceptionHandler uncaughtExceptionHandler = null; |
|
|
||
| // register a component exception handler | ||
| component.setExceptionHandler((t, e) -> { | ||
| log.error("Triggered exceptionHandler of Component: {} because of Exception in Thread: {}", |
There was a problem hiding this comment.
Since we hook up exception handler on every component, in case if multiple of them fail, we might see IllegalStateException when we try to start a thread which is already started.
There was a problem hiding this comment.
@reddycharan Even if we share the same reference across all components, multiple of them can fail at the same time, causing the hook to trigger (even though it isn't correctly implemented yet). I am fine with addressing this later since this is not very likely to happen.
There was a problem hiding this comment.
yes, as we discussed it would be shared instance.
multiple of them can fail at the same time, causing the hook to trigger
All we should care about is 'atleast-once' semantics. As long as 'shutdownHookThread' is executed once we should be fine. Anyhow in LifecycleComponentStack.start Components are started sequentially, so multiple of them failing at the same time is not going to happen.
There was a problem hiding this comment.
All we should care about is 'atleast-once' semantics. As long as 'shutdownHookThread' is executed once we should be fine.
Yes this is not a major concern, all I was saying was that it would throw a bunch of IllegalThreadStateException at the end of logs, which can be potentially confusing.
Anyhow in LifecycleComponentStack.start Components are started sequentially, so multiple of them failing at the same time is not going to happen.
It's not just during the start. Since we are passing the handler to the component, component may trigger it anytime during its lifecycle. It's OK to be not overthink much about it for now.
| } catch (UnavailableException e) { | ||
| throw new RuntimeException("Can't not start '" + NAME + "' component.", e); | ||
| } catch (Throwable exc) { | ||
| LOG.error("Got unexpected exception while starting AutoRecoveryMain", exc); |
There was a problem hiding this comment.
Lets move this to AbstractLifecycle#start() method, since exceptions can happen in any component start.
|
@reddycharan please take a look to @karanmehta93 comments |
karanmehta93
left a comment
karanmehta93
left a comment
There was a problem hiding this comment.
LGTM, Thanks for fixing this! We can merge once the checks pass too.
|
run integration tests |
|
run bookkeeper-server replication tests |
|
Good. |
|
merged to master branch, thank you @reddycharan |
3 participants
Descriptions of the changes in this PR:
When main thread of Bookie/BookieServer has exited with exception while starting Bookie,
Bookie process shouldn't be alive because of any non-daemon thread that has already
started.