Skip to content
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -918,21 +918,26 @@ public AuthenticationToken token() {
@Override
public synchronized void close() {
if (closed.compareAndSet(false, true)) {
if (zooCacheCreated.get()) {
zooCache.get().close();
}
if (zooKeeperOpened.get()) {
zooSession.get().close();

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needed to be moved to the end because it closes ZooKeeper, which deletes all ephemeral nodes and fires any watchers.

}
if (thriftTransportPool != null) {
log.debug("Closing Thrift Transport Pool");
thriftTransportPool.shutdown();
}
if (scannerReadaheadPool != null) {
log.debug("Closing Scanner ReadAhead Pool");
scannerReadaheadPool.shutdownNow(); // abort all tasks, client is shutting down
}
if (cleanupThreadPool != null) {
log.debug("Closing Cleanup ThreadPool");
cleanupThreadPool.shutdown(); // wait for shutdown tasks to execute
}
if (zooCacheCreated.get()) {
log.debug("Closing ZooCache");
zooCache.get().close();
}
if (zooKeeperOpened.get()) {
log.debug("Closing ZooSession");
zooSession.get().close();
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -82,21 +82,21 @@ public void lostLock(LockLossReason reason) {
Halt.halt(0, server + " lock in zookeeper lost (reason = " + reason
+ "), exiting cleanly because shutdown is complete.");
} else {
Halt.halt(-1, server + " lock in zookeeper lost (reason = " + reason + "), exiting!");
Halt.halt(1, server + " lock in zookeeper lost (reason = " + reason + "), exiting!");
}
}

@Override
public void unableToMonitorLockNode(final Exception e) {
Halt.halt(-1, "FATAL: No longer able to monitor " + server + " lock node", e);
Halt.halt(1, "FATAL: No longer able to monitor " + server + " lock node", e);
}

@Override
public synchronized void acquiredLock() {
LOG.debug("Acquired {} lock", server);

if (acquiredLock || failedToAcquireLock) {
Halt.halt(-1, "Zoolock in unexpected state AL " + acquiredLock + " " + failedToAcquireLock);
Halt.halt(1, "Zoolock in unexpected state AL " + acquiredLock + " " + failedToAcquireLock);
}

acquiredLock = true;
Expand All @@ -111,11 +111,11 @@ public synchronized void failedToAcquireLock(Exception e) {
String msg =
"Failed to acquire " + server + " lock due to incorrect ZooKeeper authentication.";
LOG.error("{} Ensure instance.secret is consistent across Accumulo configuration", msg, e);
Halt.halt(-1, msg);
Halt.halt(1, msg);
}

if (acquiredLock) {
Halt.halt(-1,
Halt.halt(1,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to using positive numbers.

"Zoolock in unexpected state acquiredLock true with FAL " + failedToAcquireLock);
}

Expand Down
2 changes: 1 addition & 1 deletion core/src/main/java/org/apache/accumulo/core/util/Halt.java
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public static void halt(final int status, final String msg, final Runnable runna
halt(status, msg, null, runnable);
}

public static void halt(final int status, final String msg, final Throwable exception,
private static void halt(final int status, final String msg, final Throwable exception,
final Runnable runnable) {
try {

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,24 +76,7 @@ public static interface ThriftServerSupplier {
}

public static void startServer(AbstractServer server, Logger LOG) throws Exception {
try {
server.runServer();
} catch (Throwable e) {
System.err
.println(server.getClass().getSimpleName() + " died, exception thrown from runServer.");
e.printStackTrace();
LOG.error("{} died, exception thrown from runServer.", server.getClass().getSimpleName(), e);
throw e;
} finally {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bit futher up could change the catch to catch(Exception e) instead of catch(Throwable e). The only reason it was catching Throwable was in case the finally block threw an exception.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought it was for the logging of Error, not just Exception.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was for logging an error in the case where finally threw an exception in which case the error would be lost and would not propagate. W/o the finally the error will propagate now. So it depends on what the outer code does with it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ec5cd1d

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still want to catch an exception and log here to achieve the goals of #5796? I was commenting about catching Throwable because it was only added in #5808 because of the finally. So not sure we need to catch Throwable, but may still want to catch exception.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to. I checked the logs and the Exception and Error are logged correctly. The issue in #5796 was an exception in the try-with-resources was being suppressed when an exception happened in the close. try-with-resources is no longer being used.

try {
server.close();
} catch (Throwable e) {
System.err.println("Exception thrown while closing " + server.getClass().getSimpleName());
e.printStackTrace();
LOG.error("Exception thrown while closing {}", server.getClass().getSimpleName(), e);
throw e;
}
}
server.runServer();
}

private final MetricSource metricSource;
Expand All @@ -112,6 +95,7 @@ public static void startServer(AbstractServer server, Logger LOG) throws Excepti
private volatile Thread verificationThread;
private final AtomicBoolean shutdownRequested = new AtomicBoolean(false);
private final AtomicBoolean shutdownComplete = new AtomicBoolean(false);
private final AtomicBoolean closed = new AtomicBoolean(false);

protected AbstractServer(ServerId.Type serverType, ConfigOpts opts,
Function<SiteConfiguration,ServerContext> serverContextFactory, String[] args) {
Expand Down Expand Up @@ -203,7 +187,8 @@ protected AbstractServer(ServerId.Type serverType, ConfigOpts opts,
*
* @param isIdle whether the server is idle
*/
protected void updateIdleStatus(boolean isIdle) {
// public for ExitCodesIT
public void updateIdleStatus(boolean isIdle) {
boolean shouldResetIdlePeriod = !isIdle || idleReportingPeriodMillis == 0;
boolean hasIdlePeriodStarted = idlePeriodTimer != null;
boolean hasExceededIdlePeriod =
Expand Down Expand Up @@ -290,7 +275,10 @@ public AtomicBoolean getShutdownComplete() {
*/
public void runServer() throws Exception {
final AtomicReference<Throwable> err = new AtomicReference<>();
serverThread = new Thread(TraceUtil.wrap(this), applicationName);
serverThread = new Thread(TraceUtil.wrap(() -> {
this.run();
close();
}), applicationName);
serverThread.setUncaughtExceptionHandler((thread, exception) -> err.set(exception));
serverThread.start();
serverThread.join();
Expand Down Expand Up @@ -331,7 +319,8 @@ public String getBindAddress() {
return bindAddress;
}

protected TServer getThriftServer() {
// public for ExitCodesIT
public TServer getThriftServer() {
if (thriftServer == null) {
return null;
}
Expand Down Expand Up @@ -451,7 +440,7 @@ public void startServiceLockVerificationThread() {
log.trace(
"ServiceLockVerificationThread - checking ServiceLock existence in ZooKeeper");
if (lock != null && !lock.verifyLockAtSource()) {
Halt.halt(-1, "Lock verification thread could not find lock");
Halt.halt(1, "Lock verification thread could not find lock");
}
// Need to sleep, not yield when the thread priority is greater than NORM_PRIORITY
// so that this thread does not get immediately rescheduled.
Expand All @@ -476,8 +465,21 @@ public void startServiceLockVerificationThread() {

@Override
public void close() {
if (context != null) {
context.close();

if (closed.compareAndSet(false, true)) {

// Must set shutdown as completed before calling ServerContext.close().
// ServerContext.close() calls ClientContext.close() ->
// ZooSession.close() which removes all of the ephemeral nodes and
// forces the watches to fire. The ServiceLockWatcher has a reference
// to shutdownComplete and will terminate the JVM with a 0 exit code
// if true. Otherwise it will exit with a non-zero exit code.
getShutdownComplete().set(true);

if (context != null) {
context.getLowMemoryDetector().logGCInfo(getConfiguration());
context.close();
}
}
}

Expand All @@ -488,4 +490,7 @@ protected void waitForUpgrade() throws InterruptedException {
}
}

public void requestShutdownForTests() {
shutdownRequested.set(true);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -522,9 +522,11 @@ public void close() {
getMetricsInfo().close();
}
if (sharedSchedExecutorCreated.get()) {
log.debug("Shutting down shared executor pool");
getScheduledExecutor().shutdownNow();
}
if (sharedMetadataWriterCreated.get()) {
log.debug("Shutting down shared metadata conditional writer");
try {
ConditionalWriter writer = sharedMetadataWriter.get();
if (writer != null) {
Expand All @@ -535,6 +537,7 @@ public void close() {
}
}
if (sharedUserWriterCreated.get()) {
log.debug("Shutting down shared user conditional writer");
try {
ConditionalWriter writer = sharedUserWriter.get();
if (writer != null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ public synchronized void remove(TServerInstance server) {
try {
context.getZooSession().asReaderWriter().recursiveDelete(slp.toString(), SKIP);
} catch (Exception e) {
Halt.halt(-1, "error removing tablet server lock", e);
Halt.halt(1, "error removing tablet server lock", e);
}
context.getZooCache().clear(slp.toString());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ public synchronized void logGCInfo(AccumuloConfiguration conf) {
}

if (maxIncreaseInCollectionTime > keepAliveTimeout) {
Halt.halt(-1, "Garbage collection may be interfering with lock keep-alive. Halting.");
Halt.halt(1, "Garbage collection may be interfering with lock keep-alive. Halting.");
}

localState.lastMemorySize = freeMemory;
Expand Down
Loading