issue #2879 : let bookie quit if journal thread exit

[aloyszhang]

Descriptions of the changes in this PR:
fix #2879
This pull request let bookie quit when there's journal thread exit

Motivation

As described in #2879, now if a bookie has multi journal directories means it has multi journal thread. Once a journal thread exits, the bookie will be unhealthy due to the block of all bookie-io threads, and then the bookie will not work but progress is still alive.
This pull request tries to fix this problem.

Changes

check the journal thread alive in a fixed interval, let bookie quit once there's a journal thread exit

[Vanlightly]

We don't need a new configuration for this. This kind of thing should be a hidden implementation detail, there is no benefit to allowing users to configure an interval.

Another approach would be something like passing a single CountdownLatch to each journal that can be waited on from the bookie thread.

Also note that any dependency changes need to be added to Gradle or won't compile.

[aloyszhang]

@Vanlightly
CountdownLatch sounds good. Thanks for your suggestions.

[aloyszhang]

I'm not familiar with gradle and I'll check the dependency problem first.

[aloyszhang]

@Vanlightly PTAL, thanks.

[aloyszhang]

cc @Vanlightly @eolivelli

[Vanlightly]

The JournalAliveListener approach looks good. Should it be named JournalDeathListener though? We do need to check that concurrent invocations to shutdown() either directly due to BookieServer shutdown or a journal exiting is handled. There is already handling for multiple invocations of triggerBookieShutdown, but not from that and a BookierServer shutdown. BookieServer.shutdown() should block on an already in-progress BookieImpl.shutdown() to avoid the Java process exiting too soon.

[Vanlightly]

@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.

[aloyszhang]

@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.

[Vanlightly]

@aloyszhang Exactly, if the triggerBookieShutdown happens first, which can happen due to a number of reasons, and then while it is in progress an operator initiates a controlled shutdown then the bookie can terminate before the BookieImpl shutdown is complete.

[aloyszhang]

PR Validation failed.
It seems details can be found in
file:///home/runner/work/bookkeeper/bookkeeper/bookkeeper-server/build/reports/spotbugs/main.xml, but I can't open or download this file.
@Vanlightly Can you help with this?

[Vanlightly]

@aloyszhang run this to get the report: ./gradlew bookkeeper-server:spotbugsMain

[aloyszhang]

@Vanlightly Validation issue has been resolved, PTAL, thanks!

[Vanlightly]

@aloyszhang An exception thrown during the shutdown would leave the CountdownLatch at 1, blocking the second caller. I would replace the AtomicBoolean and CountdownLatch with a simple lock, that is acquired before if (isRunning()) and that would do everything needed.

[aloyszhang]

@Vanlightly PTAL

[Vanlightly]

LGTM
@eolivelli @nicoloboschi PTAL

[aloyszhang]

CC @eolivelli PTAL, thanks!

[eolivelli]

LGTM

can you please resolve the conflicts ?
the patch is ready to be merged

[aloyszhang]

after rebasing the master branch, there're some tests that failed, I'll resolve this soon.

[dlg99]

@aloyszhang Please rebase/resolve merge conflicts

[aloyszhang]

Sorry for late, I'll do this soon

[hangc0276]

ping @aloyszhang, Would you please rebase the master?

[zymap]

@aloyszhang Could you rebase the master and address the comment?

[StevenLuMT]

this function looks nice

[StevenLuMT]

check failed,please rebase from the newest master branch

[hangc0276]

Good job!