Vanlightly commented on pull request #2887: URL: https://github.com/apache/bookkeeper/pull/2887#issuecomment-971665147
@aloyszhang I mentioned in a previous comment that we need to ensure that if a shutdown is trigger by an OS signal, but a shutdown is in progress, then it needs to wait for that shutdown to complete, else the process may exit before the shutdown is complete. I didn't see that the BookieImpl.shutdown() method is synchronized but it is, and so should do that blocking for us. Being synchronized also means that we shouldn't need that AtomicBoolean `shuttingDown` as if only one thread can execute it at a time then the `isRunning()` check is enough. However, when I tested it I found that two threads were able to enter the method concurrently! 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. The AtomicBoolean you added also isn't enough because we need an OS signal triggered shutdown to be blocked until an in-progress shutdown is completed. So that leads us to using an explicit lock on the shutdown method to replace `synchronized` and the AtomicBoolean you added. With those changes, we should have everything covered. Note that I have done a test of a crashing journal followed by an OS sigint and the bookie does terminate before the shutdown was completed, so this is something that does need addressing. -- 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]
