Vanlightly commented on a change in pull request #2887:
URL: https://github.com/apache/bookkeeper/pull/2887#discussion_r748067494
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java
##########
@@ -1155,6 +1162,7 @@ public void run() {
// cached writes durable so this is fine as well.
IOUtils.close(LOG, bc);
}
+ threadAliveCounterDown.countDown();
Review comment:
Due to programming errors, other exception types are possible. So this
might not execute. I think we should add a catch throwable above as well. Then
we know for sure any error will be logged and the countdown latch will be
decremented.
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/conf/ServerConfiguration.java
##########
@@ -308,6 +308,10 @@
// Certificate role based authorization
protected static final String AUTHORIZED_ROLES = "authorizedRoles";
+
+ // Certificate role based authorization
+ protected static final String JOURNAL_ALIVE_CHECK_INTERVAL =
"journalAliveCheckInterval";
Review comment:
This can be removed now.
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java
##########
@@ -1134,14 +1139,11 @@ public void run() {
for (Journal journal: journals) {
journal.start();
}
-
- // wait until journal quits
- for (Journal journal: journals) {
-
- journal.joinThread();
- }
+ journalExistCountDown.await();
+ // we are here means at latest one journal thread exit.
+ triggerBookieShutdown(ExitCode.BOOKIE_EXCEPTION);
Review comment:
This code is reached during a normal shutdown which is far more likely
than a journal failing. This can just get removed as there is already a check
that covers this lower down.
##########
File path: dependencies.gradle
##########
@@ -84,6 +84,7 @@ depVersions = [
vertx: "3.9.8",
yahooDatasketches: "0.8.3",
zookeeper: "3.6.2",
+ awaitility: "4.0.2",
Review comment:
This should be alphabetical.
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java
##########
@@ -1179,12 +1196,12 @@ public void run() {
public int shutdown() {
return shutdown(ExitCode.OK);
}
-
+ private final AtomicBoolean shuttingDown = new AtomicBoolean(false);
Review comment:
There is already the shutdownTriggered AtomicBoolean which performs a
similar role. Another thing to consider is that when shutdown is called due to
an OS signal, we need to make sure concurrent invocations of
BookieImpl.shutdown() are handled correctly. For example, if the bookie is
shutting down due to an OS signal, it should wait for an in-progress shutdown
to complete before exiting.
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java
##########
@@ -715,13 +716,23 @@ public BookieImpl(ServerConfiguration conf, StatsLogger
statsLogger,
}
}
- // instantiate the journals
journals = Lists.newArrayList();
- for (int i = 0; i < journalDirectories.size(); i++) {
- journals.add(new Journal(i, journalDirectories.get(i),
- conf, ledgerDirsManager, statsLogger.scope(JOURNAL_SCOPE),
allocator));
+ if (listenJournalAlive) {
Review comment:
Do we need this? Can't we always set up the JournalAliveListener?
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BookieImpl.java
##########
@@ -1134,14 +1139,11 @@ public void run() {
for (Journal journal: journals) {
journal.start();
}
-
- // wait until journal quits
- for (Journal journal: journals) {
-
- journal.joinThread();
- }
+ journalExistCountDown.await();
+ // we are here means at latest one journal thread exit.
+ triggerBookieShutdown(ExitCode.BOOKIE_EXCEPTION);
LOG.info("Journal thread(s) quit.");
- } catch (InterruptedException ie) {
+ } catch (Throwable ie) {
Review comment:
Is there any type of exception that can occur here, and if so, why log
it as an InterruptedException?
##########
File path: dependencies.gradle
##########
@@ -174,6 +175,7 @@ depLibs = [
metricsJvm: "io.dropwizard.metrics:metrics-jvm:${depVersions.dropwizard}",
metricsGraphite:
"io.dropwizard.metrics:metrics-graphite:${depVersions.dropwizard}",
mockito: "org.mockito:mockito-core:${depVersions.mockito}",
+ awaitility: "org.awaitility:awaitility:${depVersions.awaitility}",
Review comment:
This should be alphabetical.
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/Journal.java
##########
@@ -702,6 +704,67 @@ public Journal(int journalIndex, File journalDirectory,
ServerConfiguration conf
this.journalStats = new JournalStats(statsLogger);
}
+ public Journal(int journalIndex, File journalDirectory,
ServerConfiguration conf,
Review comment:
You don't need to duplicate a whole constructor for this.
--
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]
