aloyszhang edited a comment on pull request #2887: URL: https://github.com/apache/bookkeeper/pull/2887#issuecomment-972577775
@Vanlightly > This seems due to the this.join() as that is the moment when another thread is permitted to enter the method. So synchronized isn't doing the job for us. Yes, the original `synchronized ` does not work due to the join(). > Note that I have done a test of a crashing journal followed by an OS signal and the bookie does terminate before the shutdown was completed, so this is something that does need addressing. Do you mean that there is a common shutdown in progress which invoked by the stop command `bookkeeper-deamon stop bookie`, at the same time, the journal thread crashed due to os failure, and the bookie server will shutdown without waiting for the common shutdown to complete. I think this can't happen. Because the common shutdown will trigger `ComponentShutdownHook` which invokes `BookieImpl.shutdown()` directly, and if journal crash at the same time, this will trigger a shutdown by `BookieImpl.triggerBookieShutdown()` which will go into `BookieImpl.shutdown()` but execute nothing since `shuttingDown` flag is already set to true. I think the bad case is that if a journal crash first and already triggers `BookieImpl.triggerBookieShutdown()`, then we shutdown bookie by the command `bookkeeper-deamon stop bookie`, the shutdown progress by the command may not execute completely. And we have add another controll to make sure the graceful shutdown will complete. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
