This is an automated email from the ASF dual-hosted git repository.
eolivelli pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/bookkeeper.git
The following commit(s) were added to refs/heads/master by this push:
new 114de13 Call exceptionHandler if Bookie.start fails with exception.
114de13 is described below
commit 114de135f402d2f8de045fb9413e804247841def
Author: Charan Reddy Guttapalem <[email protected]>
AuthorDate: Fri Oct 11 08:22:54 2019 -0700
Call exceptionHandler if Bookie.start fails with exception.
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.
Reviewers: Enrico Olivelli <[email protected]>, Karan Mehta
<[email protected]>
This closes #2173 from reddycharan/fixbookiestartup
---
.../component/AbstractLifecycleComponent.java | 16 +++-
.../common/component/ComponentStarter.java | 5 +-
.../org/apache/bookkeeper/proto/BookieServer.java | 3 +-
.../bookkeeper/replication/AutoRecoveryMain.java | 2 +-
.../server/service/AutoRecoveryService.java | 8 +-
.../bookkeeper/server/service/BookieService.java | 7 +-
.../bookie/BookieInitializationTest.java | 93 +++++++++++++++++++++-
.../bookkeeper/bookie/BookieJournalTest.java | 6 +-
8 files changed, 120 insertions(+), 20 deletions(-)
diff --git
a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java
b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java
index 015d54d..5b6f19e 100644
---
a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java
+++
b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/AbstractLifecycleComponent.java
@@ -25,6 +25,8 @@ import java.util.concurrent.CopyOnWriteArraySet;
import lombok.extern.slf4j.Slf4j;
import org.apache.bookkeeper.common.conf.ComponentConfiguration;
import org.apache.bookkeeper.stats.StatsLogger;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* A mix of {@link AbstractComponent} and {@link LifecycleComponent}.
@@ -33,6 +35,8 @@ import org.apache.bookkeeper.stats.StatsLogger;
public abstract class AbstractLifecycleComponent<ConfT extends
ComponentConfiguration>
extends AbstractComponent<ConfT> implements LifecycleComponent {
+ private static final Logger LOG =
LoggerFactory.getLogger(AbstractLifecycleComponent.class);
+
protected final Lifecycle lifecycle = new Lifecycle();
private final Set<LifecycleListener> listeners = new
CopyOnWriteArraySet<>();
protected final StatsLogger statsLogger;
@@ -75,7 +79,17 @@ public abstract class AbstractLifecycleComponent<ConfT
extends ComponentConfigur
return;
}
listeners.forEach(LifecycleListener::beforeStart);
- doStart();
+ try {
+ doStart();
+ } catch (Throwable exc) {
+ LOG.error("Failed to start Component: {}", getName(), exc);
+ if (uncaughtExceptionHandler != null) {
+ LOG.error("Calling uncaughtExceptionHandler");
+
uncaughtExceptionHandler.uncaughtException(Thread.currentThread(), exc);
+ } else {
+ throw exc;
+ }
+ }
lifecycle.moveToStarted();
listeners.forEach(LifecycleListener::afterStart);
}
diff --git
a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java
b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java
index e952747..acb643b 100644
---
a/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java
+++
b/bookkeeper-common/src/main/java/org/apache/bookkeeper/common/component/ComponentStarter.java
@@ -19,6 +19,7 @@
package org.apache.bookkeeper.common.component;
import java.util.concurrent.CompletableFuture;
+
import lombok.extern.slf4j.Slf4j;
import org.apache.bookkeeper.common.concurrent.FutureUtils;
@@ -46,7 +47,7 @@ public class ComponentStarter {
component.close();
log.info("Closed component {} in shutdown hook successfully.
Exiting.", component.getName());
FutureUtils.complete(future, null);
- } catch (Exception e) {
+ } catch (Throwable e) {
log.error("Failed to close component {} in shutdown hook
gracefully, Exiting anyway",
component.getName(), e);
future.completeExceptionally(e);
@@ -72,6 +73,8 @@ public class ComponentStarter {
// 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();
});
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
index b8c1adc..14d6393 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/proto/BookieServer.java
@@ -39,7 +39,6 @@ import org.apache.bookkeeper.bookie.BookieCriticalThread;
import org.apache.bookkeeper.bookie.BookieException;
import org.apache.bookkeeper.bookie.ExitCode;
import org.apache.bookkeeper.bookie.ReadOnlyBookie;
-import org.apache.bookkeeper.client.BKException;
import org.apache.bookkeeper.common.allocator.ByteBufAllocatorBuilder;
import org.apache.bookkeeper.common.util.JsonUtil.ParseJsonException;
import org.apache.bookkeeper.conf.ServerConfiguration;
@@ -137,7 +136,7 @@ public class BookieServer {
: new Bookie(conf, statsLogger.scope(BOOKIE_SCOPE), allocator);
}
- public void start() throws IOException, UnavailableException,
InterruptedException, BKException {
+ public void start() throws InterruptedException {
this.bookie.start();
// fail fast, when bookie startup is not successful
if (!this.bookie.isRunning()) {
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java
index 39b41aa..303a401 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/replication/AutoRecoveryMain.java
@@ -116,7 +116,7 @@ public class AutoRecoveryMain {
/*
* Start daemons
*/
- public void start() throws UnavailableException {
+ public void start() {
auditorElector.start();
replicationWorker.start();
if (null != uncaughtExceptionHandler) {
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/AutoRecoveryService.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/AutoRecoveryService.java
index f8389df..c7305c2 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/AutoRecoveryService.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/AutoRecoveryService.java
@@ -22,7 +22,6 @@ import java.io.IOException;
import java.lang.Thread.UncaughtExceptionHandler;
import org.apache.bookkeeper.replication.AutoRecoveryMain;
-import
org.apache.bookkeeper.replication.ReplicationException.UnavailableException;
import org.apache.bookkeeper.server.component.ServerLifecycleComponent;
import org.apache.bookkeeper.server.conf.BookieConfiguration;
import org.apache.bookkeeper.stats.StatsLogger;
@@ -45,6 +44,7 @@ public class AutoRecoveryService extends
ServerLifecycleComponent {
@Override
public void setExceptionHandler(UncaughtExceptionHandler handler) {
+ super.setExceptionHandler(handler);
main.setExceptionHandler(handler);
}
@@ -54,11 +54,7 @@ public class AutoRecoveryService extends
ServerLifecycleComponent {
@Override
protected void doStart() {
- try {
- this.main.start();
- } catch (UnavailableException e) {
- throw new RuntimeException("Can't not start '" + NAME + "'
component.", e);
- }
+ this.main.start();
}
@Override
diff --git
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/BookieService.java
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/BookieService.java
index d6837f6..645da03 100644
---
a/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/BookieService.java
+++
b/bookkeeper-server/src/main/java/org/apache/bookkeeper/server/service/BookieService.java
@@ -20,9 +20,7 @@ package org.apache.bookkeeper.server.service;
import java.io.IOException;
import java.lang.Thread.UncaughtExceptionHandler;
-import org.apache.bookkeeper.client.BKException;
import org.apache.bookkeeper.proto.BookieServer;
-import
org.apache.bookkeeper.replication.ReplicationException.UnavailableException;
import org.apache.bookkeeper.server.component.ServerLifecycleComponent;
import org.apache.bookkeeper.server.conf.BookieConfiguration;
import org.apache.bookkeeper.stats.StatsLogger;
@@ -45,6 +43,7 @@ public class BookieService extends ServerLifecycleComponent {
@Override
public void setExceptionHandler(UncaughtExceptionHandler handler) {
+ super.setExceptionHandler(handler);
server.setExceptionHandler(handler);
}
@@ -56,8 +55,8 @@ public class BookieService extends ServerLifecycleComponent {
protected void doStart() {
try {
this.server.start();
- } catch (IOException | UnavailableException | InterruptedException |
BKException e) {
- throw new RuntimeException("Failed to start bookie server", e);
+ } catch (InterruptedException exc) {
+ throw new RuntimeException("Failed to start bookie server", exc);
}
}
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
index 8ea63e1..77bb0bd 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieInitializationTest.java
@@ -24,9 +24,13 @@ import static com.google.common.base.Charsets.UTF_8;
import static org.apache.bookkeeper.util.BookKeeperConstants.AVAILABLE_NODE;
import static
org.apache.bookkeeper.util.BookKeeperConstants.BOOKIE_STATUS_FILENAME;
import static org.apache.bookkeeper.util.TestUtils.countNumOfFiles;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.hasItem;
+import static org.hamcrest.Matchers.hasProperty;
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
+import static org.junit.Assert.assertThat;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;
import static org.mockito.ArgumentMatchers.anyBoolean;
@@ -35,6 +39,8 @@ import static org.mockito.Mockito.doThrow;
import static org.mockito.Mockito.mock;
import com.fasterxml.jackson.databind.ObjectMapper;
+
+import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufAllocator;
import io.netty.buffer.UnpooledByteBufAllocator;
@@ -58,6 +64,8 @@ import java.util.Random;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;
+import java.util.concurrent.atomic.AtomicInteger;
+
import
org.apache.bookkeeper.bookie.BookieException.DiskPartitionDuplicationException;
import org.apache.bookkeeper.bookie.BookieException.MetadataStoreException;
import org.apache.bookkeeper.bookie.Journal.LastLogMark;
@@ -95,6 +103,7 @@ import org.apache.bookkeeper.test.BookKeeperClusterTestCase;
import org.apache.bookkeeper.test.PortManager;
import org.apache.bookkeeper.tls.SecurityException;
import org.apache.bookkeeper.util.DiskChecker;
+import org.apache.bookkeeper.util.LoggerOutput;
import org.apache.bookkeeper.zookeeper.ZooKeeperClient;
import org.apache.zookeeper.KeeperException;
import org.apache.zookeeper.data.Stat;
@@ -105,6 +114,7 @@ import org.junit.rules.TestName;
import org.powermock.reflect.Whitebox;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import org.slf4j.event.LoggingEvent;
/**
* Testing bookie initialization cases.
@@ -117,6 +127,8 @@ public class BookieInitializationTest extends
BookKeeperClusterTestCase {
@Rule
public final TestName runtime = new TestName();
+ @Rule
+ public LoggerOutput loggerOutput = new LoggerOutput();
ZKMetadataBookieDriver driver;
public BookieInitializationTest() {
@@ -640,6 +652,85 @@ public class BookieInitializationTest extends
BookKeeperClusterTestCase {
startFuture.get();
}
+ /**
+ * Mock InterleavedLedgerStorage class where addEntry is mocked to throw
+ * OutOfMemoryError.
+ */
+ public static class MockInterleavedLedgerStorage extends
InterleavedLedgerStorage {
+ AtomicInteger atmoicInt = new AtomicInteger(0);
+
+ @Override
+ public long addEntry(ByteBuf entry) throws IOException {
+ if (atmoicInt.incrementAndGet() == 10) {
+ throw new OutOfMemoryError("Some Injected Exception");
+ }
+ return super.addEntry(entry);
+ }
+ }
+
+ @Test
+ public void testBookieStartException() throws Exception {
+ File journalDir = createTempDir("bookie", "journal");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(journalDir));
+
+ File ledgerDir = createTempDir("bookie", "ledger");
+ Bookie.checkDirectoryStructure(Bookie.getCurrentDirectory(ledgerDir));
+
+ /*
+ * add few entries to journal file.
+ */
+ int numOfEntries = 100;
+
BookieJournalTest.writeV5Journal(Bookie.getCurrentDirectory(journalDir),
numOfEntries,
+ "testV5Journal".getBytes());
+
+ /*
+ * This Bookie is configured to use MockInterleavedLedgerStorage.
+ * MockInterleavedLedgerStorage throws an Error for addEntry request.
+ * This is to simulate Bookie/BookieServer/BookieService 'start'
failure
+ * because of 'Bookie.readJournal' failure.
+ */
+ ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
+ int port = PortManager.nextFreePort();
+ conf.setBookiePort(port).setJournalDirName(journalDir.getPath())
+ .setLedgerDirNames(new String[] { ledgerDir.getPath()
}).setMetadataServiceUri(metadataServiceUri)
+
.setLedgerStorageClass(MockInterleavedLedgerStorage.class.getName());
+
+ BookieConfiguration bkConf = new BookieConfiguration(conf);
+
+ /*
+ * create cookie and write it to JournalDir/LedgerDir.
+ */
+ Cookie.Builder cookieBuilder = Cookie.generateCookie(conf);
+ Cookie cookie = cookieBuilder.build();
+ cookie.writeToDirectory(new File(journalDir, "current"));
+ cookie.writeToDirectory(new File(ledgerDir, "current"));
+
+ /*
+ * Create LifecycleComponent for BookieServer and start it.
+ */
+ LifecycleComponent server = Main.buildBookieServer(bkConf);
+ CompletableFuture<Void> startFuture =
ComponentStarter.startComponent(server);
+
+ /*
+ * Since Bookie/BookieServer/BookieService is expected to fail, it
would
+ * cause bookie-server component's exceptionHandler to get triggered.
+ * This exceptionHandler will make sure all of the components to get
+ * closed and then finally completes the Future.
+ */
+ startFuture.get();
+
+ /*
+ * make sure that Component's exceptionHandler is called by checking if
+ * the error message of ExceptionHandler is logged. This Log message is
+ * defined in anonymous exceptionHandler class defined in
+ * ComponentStarter.startComponent method.
+ */
+ loggerOutput.expect((List<LoggingEvent> logEvents) -> {
+ assertThat(logEvents,
+ hasItem(hasProperty("message", containsString("Triggered
exceptionHandler of Component:"))));
+ });
+ }
+
@Test
public void testAutoRecoveryServiceExceptionHandler() throws Exception {
ServerConfiguration conf =
TestBKConfiguration.newServerConfiguration();
@@ -889,8 +980,6 @@ public class BookieInitializationTest extends
BookKeeperClusterTestCase {
// are injecting no-op shutdown.
server.shutdown();
- long usableSpace = tmpDir.getUsableSpace();
- long totalSpace = tmpDir.getTotalSpace();
conf.setDiskUsageThreshold(0.001f)
.setDiskUsageWarnThreshold(0.0f).setReadOnlyModeEnabled(true).setIsForceGCAllowWhenNoSpace(true)
.setMinUsableSizeForIndexFileCreation(Long.MAX_VALUE);
diff --git
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
index 0f1b327..cbac455 100644
---
a/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
+++
b/bookkeeper-server/src/test/java/org/apache/bookkeeper/bookie/BookieJournalTest.java
@@ -111,7 +111,7 @@ public class BookieJournalTest {
/**
* Generate fence entry.
*/
- private ByteBuf generateFenceEntry(long ledgerId) {
+ private static ByteBuf generateFenceEntry(long ledgerId) {
ByteBuf bb = Unpooled.buffer();
bb.writeLong(ledgerId);
bb.writeLong(Bookie.METAENTRY_ID_FENCE_KEY);
@@ -121,7 +121,7 @@ public class BookieJournalTest {
/**
* Generate meta entry with given master key.
*/
- private ByteBuf generateMetaEntry(long ledgerId, byte[] masterKey) {
+ private static ByteBuf generateMetaEntry(long ledgerId, byte[] masterKey) {
ByteBuf bb = Unpooled.buffer();
bb.writeLong(ledgerId);
bb.writeLong(Bookie.METAENTRY_ID_LEDGER_KEY);
@@ -290,7 +290,7 @@ public class BookieJournalTest {
return jc;
}
- private JournalChannel writeV5Journal(File journalDir, int numEntries,
byte[] masterKey) throws Exception {
+ static JournalChannel writeV5Journal(File journalDir, int numEntries,
byte[] masterKey) throws Exception {
long logId = System.currentTimeMillis();
JournalChannel jc = new JournalChannel(journalDir, logId);
