ctubbsii commented on code in PR #5574:
URL: https://github.com/apache/accumulo/pull/5574#discussion_r2103805467
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockSupport.java:
##########
@@ -63,9 +63,8 @@ public void lostLock(LockLossReason reason) {
@Override
public void unableToMonitorLockNode(final Exception e) {
// ACCUMULO-3651 Changed level to error and added FATAL to message for
slf4j compatibility
- Halt.halt(-1,
- () -> LOG.error("FATAL: No longer able to monitor {} lock node",
serviceName, e));
-
+ Halt.halt(-1, "FATAL: No longer able to monitor " + serviceName + " lock
node",
+ () -> e.printStackTrace());
Review Comment:
I'm not sure the comment above this makes any sense anymore.
```suggestion
Halt.halt(-1, "FATAL: No longer able to monitor " + serviceName + "
lock node",
e::printStackTrace);
```
##########
core/src/main/java/org/apache/accumulo/core/util/Halt.java:
##########
@@ -30,26 +30,20 @@ public class Halt {
public static void halt(final String msg) {
// ACCUMULO-3651 Changed level to error and added FATAL to message for
slf4j compatibility
- halt(0, new Runnable() {
- @Override
- public void run() {
- log.error("FATAL {}", msg);
- }
- });
+ halt(0, "FATAL " + msg, null);
}
public static void halt(final String msg, int status) {
- halt(status, new Runnable() {
- @Override
- public void run() {
- log.error("FATAL {}", msg);
- }
- });
+ halt(status, "FATAL " + msg, null);
}
- public static void halt(final int status, Runnable runnable) {
+ public static void halt(final int status, String msg, Runnable runnable) {
try {
+ // Printing to stderr and to the log in case the message does not make
+ // it to the log. This could happen if an asynchronous logging impl is
used
+ System.err.println(msg);
Review Comment:
I'm thinking that we should probably flush STDERR. I've seen processes die
too quickly for it to get flushed on its own, and the message sent to
System.err never appears.
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletClientHandler.java:
##########
@@ -1155,11 +1155,11 @@ static void checkPermission(ServerContext context,
TabletHostingServer server,
log.trace("Got {} message from user: {}", request,
credentials.getPrincipal());
if (server.getLock() != null && server.getLock().wasLockAcquired()
&& !server.getLock().isLocked()) {
- Halt.halt(1, () -> {
- log.info("Tablet server no longer holds lock during checkPermission()
: {}, exiting",
- request);
- server.getGcLogger().logGCInfo(server.getConfiguration());
- });
+ Halt.halt(1,
+ "Tablet server no longer holds lock during checkPermission() : " +
request + ", exiting",
+ () -> {
+ server.getGcLogger().logGCInfo(server.getConfiguration());
+ });
Review Comment:
```suggestion
() -> server.getGcLogger().logGCInfo(server.getConfiguration()));
```
##########
server/base/src/main/java/org/apache/accumulo/server/util/FileSystemMonitor.java:
##########
@@ -121,8 +121,8 @@ public FileSystemMonitor(final String procFile, long
period, AccumuloConfigurati
try {
checkMount(mount);
} catch (final Exception e) {
- Halt.halt(-42,
- () -> log.error("Exception while checking mount points,
halting process", e));
+ Halt.halt(-42, "Exception while checking mount points, halting
process",
Review Comment:
Negative 42 is a little crazy. I'm not sure where these numbers come from.
But, we really shouldn't be using negative values for exit codes. Some
environments don't support that. Positive 42 is less crazy (but still strange
and arbitrary).
##########
core/src/main/java/org/apache/accumulo/core/util/Halt.java:
##########
@@ -30,25 +30,19 @@ public class Halt {
public static void halt(final String msg) {
// ACCUMULO-3651 Changed level to error and added FATAL to message for
slf4j compatibility
- halt(0, new Runnable() {
- @Override
- public void run() {
- log.error("FATAL {}", msg);
- }
- });
+ halt(0, "FATAL " + msg, null);
}
public static void halt(final String msg, int status) {
- halt(status, new Runnable() {
- @Override
- public void run() {
- log.error("FATAL {}", msg);
- }
- });
+ halt(status, "FATAL " + msg, null);
}
- public static void halt(final int status, Runnable runnable) {
+ public static void halt(final int status, String msg, Runnable runnable) {
Review Comment:
The optional exception might still be a good idea as a separate parameter,
so we're not using the Runnable action for simply passing an exception. Since
we're still sending it to the logging framework, it's better to pass the
exception, rather than omit it from the logged error and rely on the Runnable
to print the stack trace This method can print the stack trace, if an exception
is passed, and also give it to the log framework.
##########
server/base/src/main/java/org/apache/accumulo/server/util/FileSystemMonitor.java:
##########
@@ -121,8 +121,8 @@ public FileSystemMonitor(final String procFile, long
period, AccumuloConfigurati
try {
checkMount(mount);
} catch (final Exception e) {
- Halt.halt(-42,
- () -> log.error("Exception while checking mount points,
halting process", e));
+ Halt.halt(-42, "Exception while checking mount points, halting
process",
+ () -> e.printStackTrace());
Review Comment:
```suggestion
e::printStackTrace);
```
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLockSupport.java:
##########
@@ -145,16 +144,15 @@ public void lostLock(final LockLossReason reason) {
LOG.warn("{} lost lock (reason = {}), not halting because shutdown is
complete.",
serviceName, reason);
} else {
- Halt.halt(1, () -> {
- LOG.error("{} lost lock (reason = {}), exiting.", serviceName,
reason);
- lostLockAction.accept(serviceName);
- });
+ Halt.halt(1, serviceName + " lost lock (reason = " + reason + "),
exiting.",
+ () -> lostLockAction.accept(serviceName));
}
}
@Override
public void unableToMonitorLockNode(final Exception e) {
- Halt.halt(1, () -> LOG.error("Lost ability to monitor {} lock,
exiting.", serviceName, e));
+ Halt.halt(1, "Lost ability to monitor " + serviceName + " lock,
exiting.",
+ () -> e.printStackTrace());
Review Comment:
```suggestion
e::printStackTrace);
```
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
