Fix bookie process still alive when bookie startup is not successful and shutdown

[TakaHiR07]

Motivation

When bookie startup encounter IOException in BookieImpl#readJournal(), bookie startup is not successful and then trigger shutdown(). However, bookie process is still alive.

The reason is IOException is caught in BookieImpl and trigger shutdown. Bookie actually is not running. But the exception do not throw to startComponent.

So BookieServer.main / server.Main.doMain still wait for the startComponent future to complete.

The relevant code is as following:

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java

Lines 654 to 660 in 3221aa3

	try {
	readJournal();
	} catch (IOException | BookieException ioe) {
	LOG.error("Exception while replaying journals, shutting down", ioe);
	shutdown(ExitCode.BOOKIE_EXCEPTION);
	return;
	}

bookkeeper/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java

Lines 123 to 131 in 3221aa3

	public void start() throws InterruptedException, IOException {
	this.bookie.start();
	// fail fast, when bookie startup is not successful
	if (!this.bookie.isRunning()) {
	exitCode = bookie.getExitCode();
	this.requestProcessor.close();
	return;
	}

bookkeeper/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java

Lines 63 to 87 in 3221aa3

	public static CompletableFuture<Void> startComponent(LifecycleComponent component) {
	CompletableFuture<Void> future = new CompletableFuture<>();
	final Thread shutdownHookThread = new Thread(
	new ComponentShutdownHook(component, future),
	"component-shutdown-thread"
	);
	// register a shutdown hook
	Runtime.getRuntime().addShutdownHook(shutdownHookThread);
	// register a component exception handler
	component.setExceptionHandler((t, e) -> {
	log.error("Triggered exceptionHandler of Component: {} because of Exception in Thread: {}",
	component.getName(), t, e);
	// start the shutdown hook when an uncaught exception happen in the lifecycle component.
	shutdownHookThread.start();
	});
	component.publishInfo(new ComponentInfoPublisher());
	log.info("Starting component {}.", component.getName());
	component.start();
	log.info("Started component {}.", component.getName());
	return future;
	}

Changes

throw exception to startComponent when bookie start encounter error.

[hangc0276]

We'd better assert startFuture and startFuture2 separately.

[TakaHiR07]

have done.

[horizonzy]

We would better define another Exception; the InterruptedException is for the thread case.

[TakaHiR07]

Can we define it "BookieStartException", extends the BookieException ?

[horizonzy]

Sounds good.

[TakaHiR07]

have done.

[hangc0276]

@TakaHiR07 Could you take a look at the check style issue? Thanks.

[TakaHiR07]

@TakaHiR07 Could you take a look at the check style issue? Thanks.

have fix checkstyle.

[horizonzy]

LGTM.

[hangc0276]

@TakaHiR07 Would you please help fix the failed tests? Thanks.

[TakaHiR07]

@TakaHiR07 Would you please help fix the failed tests? Thanks.

@hangc0276 I have fix the test since powermock is removed in master branch.

[shoothzj]

update the branch

[xiang092689]

Normally, "Main" thread will wait for "component-shutdown-thread" thread to complete the "component start future". Your edition will cause future complete too soon, and main thread exit, which may cause shutdown not finished and process exit?

[shoothzj]

I think it's not an APIException, should we define this exception in https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ExitCode.java ?

[TakaHiR07]

Normally, "Main" thread will wait for "component-shutdown-thread" thread to complete the "component start future". Your edition will cause future complete too soon, and main thread exit, which may cause shutdown not finished and process exit?

"component start future" would complete only when exception throw. if bookie.start() encounter exception, it may catch some exception and go into shutdown(). only shutdown finish, then bookie.isRunning() become false, then go into the edition code.

[TakaHiR07]

I think it's not an APIException, should we define this exception in https://github.com/apache/bookkeeper/blob/master/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/ExitCode.java ?

ExitCode looks like pointing out the exact reason why bookie start fail. It seems not appropriate to put this exception here, this exception only point out that bookie start fail.

[TakaHiR07]

Many failed test shows that in previous design, bookieServer start failed, but not throw exception. I think these test is also unreasonable and should be fixed. Looking forward to more views, then I would fix these test.