[
https://issues.apache.org/jira/browse/ACCUMULO-3652?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=14526708#comment-14526708
]
Josh Elser commented on ACCUMULO-3652:
--------------------------------------
Lots of incorrect changes which add in braces where they shouldn't be WRT
exceptions. I'll stop commenting when I see them (about 50% of the way through
the patch)
{code}
@@ -139,7 +139,7 @@ public class TCredentialsUpdatingInvocationHandler<I>
implements InvocationHandl
try {
clz = Class.forName(tokenClassName);
} catch (ClassNotFoundException e) {
- log.debug("Could not create class from token name: " + tokenClassName,
e);
+ log.debug("Could not create class from token name: {} {}",
tokenClassName, e);
{code}
Extra set of braces.
{code}
@@ -185,7 +185,7 @@ public class TabletStateChangeIterator extends
SkippingIterator {
boolean shouldBeOnline =
onlineTables.contains(tls.extent.getTableId().toString());
if (debug) {
- log.debug(tls.extent + " is " + tls.getState(current) + " and should
be " + (shouldBeOnline ? "on" : "off") + "line");
+ log.debug("{} is {} and should be {} line", tls.extent,
tls.getState(current), (shouldBeOnline ? "on" : "off"));
{code}
Extra space between the third pair of braces and "line"
{code}
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java
b/server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java
index e27a7e7..bf5ba8f 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/security/SecurityUtil.java
@@ -72,13 +72,13 @@ public class SecurityUtil {
....
- log.error("Error logging in user " + principalConfig + " using keytab at
" + keyTabPath, io);
+ log.error("Error logging in user {} using keytab at {} {}",
principalConfig, keyTabPath, io);
{code}
Extra set of braces.
{code}
diff --git
a/server/base/src/main/java/org/apache/accumulo/server/util/VerifyTabletAssignments.java
b/server/base/src/main/java/org/apache/accumulo/server/util/VerifyTabletAssignments.java
index c0c979b..3775fc8 100644
---
a/server/base/src/main/java/org/apache/accumulo/server/util/VerifyTabletAssignments.java
+++
b/server/base/src/main/java/org/apache/accumulo/server/util/VerifyTabletAssignments.java
@@ -126,7 +126,7 @@ public class VerifyTabletAssignments {
try {
checkTabletServer(context, entry, failures);
} catch (Exception e) {
- log.error("Failure on tablet server '" + entry.getKey() + ".", e);
+ log.error("Failure on tablet server ' {}. {}", entry.getKey(), e);
{code}
Extra set of braces
{code}
@@ -225,7 +226,8 @@ public class GarbageCollectWriteAheadLogs {
} catch (FileNotFoundException ex) {
// ignored
} catch (IOException ex) {
- log.error("Unable to delete wal " + path + ": " + ex);
+ //log.error("Unable to check for the existence of {} {}", swalog,
ex);
+ log.error("Unable to delete wal {}",path , ex);
{code}
Nit: remove the commented code.
{code}
@@ -338,7 +340,7 @@ public class GarbageCollectWriteAheadLogs {
return true;
}
} catch (InvalidProtocolBufferException e) {
- log.error("Could not deserialize Status protobuf for " +
entry.getKey(), e);
+ log.error("Could not deserialize Status protobuf for {} {}",
entry.getKey(), e);
{code}
Extra set of braces
{code}
@@ -366,13 +366,13 @@ public class SimpleGarbageCollector extends
AccumuloServerContext implements Ifa
// atomically in one mutation and extreme care would need to
be taken that delete entry was not lost. Instead of doing that, just deal with
// volume switching when something needs to be deleted. Since
the rest of the code uses suffixes to compare delete entries, there is no danger
// of deleting something that should not be deleted. Must not
change value of delete variable because thats whats stored in metadata table.
- log.debug("Volume replaced " + delete + " -> " +
switchedDelete);
+ log.debug("Volume replaced {} -> ", delete, switchedDelete);
{code}
Missing a pair of braces
{code}
@@ -1071,7 +1070,7 @@ public class Master extends AccumuloServerContext
implements LiveTServerSet.List
} catch (TTransportException e) {
// ignore: it's probably down
} catch (Exception e) {
- log.info("error talking to troublesome tablet server ", e);
+ log.info("error talking to troublesome tablet server {}", e);
}
badServers.remove(server);
tserverSet.remove(server);
@@ -1113,7 +1112,7 @@ public class Master extends AccumuloServerContext
implements LiveTServerSet.List
// watcher only fires once, add it back
ZooReaderWriter.getInstance().getChildren(zroot +
Constants.ZRECOVERY, this);
} catch (Exception e) {
- log.error("Failed to add log recovery watcher back", e);
+ log.error("Failed to add log recovery watcher back {}", e);
}
}
});
{code}
Two sets of extra braces.
{code}
@@ -1199,7 +1198,7 @@ public class Master extends AccumuloServerContext
implements LiveTServerSet.List
try {
replicationWorkAssigner = new WorkDriver(this);
} catch (AccumuloException | AccumuloSecurityException e) {
- log.error("Caught exception trying to initialize replication
WorkDriver", e);
+ log.error("Caught exception trying to initialize replication WorkDriver
{}", e);
{code}
Extra braces
{code}
@@ -1223,7 +1222,7 @@ public class Master extends AccumuloServerContext
implements LiveTServerSet.List
try {
replicationMetrics.register();
} catch (Exception e) {
- log.error("Failed to register replication metrics", e);
+ log.error("Failed to register replication metrics {}", e);
}
while (clientService.isServing()) {
@@ -1275,7 +1274,7 @@ public class Master extends AccumuloServerContext
implements LiveTServerSet.List
Halt.halt(-1, new Runnable() {
@Override
public void run() {
- log.error("FATAL: No longer able to monitor master lock node", e);
+ log.error("FATAL: No longer able to monitor master lock node {}", e);
}
});
@@ -1295,7 +1294,7 @@ public class Master extends AccumuloServerContext
implements LiveTServerSet.List
@Override
public synchronized void failedToAcquireLock(Exception e) {
- log.warn("Failed to get master lock " + e);
+ log.warn("Failed to get master lock {}", e);
if (e instanceof NoAuthException) {
String msg = "Failed to acquire master lock due to incorrect ZooKeeper
authentication.";
@@ -1365,7 +1364,7 @@ public class Master extends AccumuloServerContext
implements LiveTServerSet.List
DistributedTrace.enable(hostname, app, conf.getConfiguration());
master.run();
} catch (Exception ex) {
- log.error("Unexpected exception, exiting", ex);
+ log.error("Unexpected exception, exiting {}", ex);
System.exit(1);
} finally {
DistributedTrace.disable();
{code}
More extra braces
{code}
@@ -323,7 +325,7 @@ class TabletGroupWatcher extends Daemon {
Master.log.info("Detected change in current tserver set, re-running
state machine.");
}
} catch (Exception ex) {
- Master.log.error("Error processing table state for store " +
store.name(), ex);
+ Master.log.error("Error processing table state for store {} {}",
store.name(), ex);
{code}
Extra braces
{code}
@@ -334,7 +336,7 @@ class TabletGroupWatcher extends Daemon {
try {
iter.close();
} catch (IOException ex) {
- Master.log.warn("Error closing TabletLocationState iterator: " +
ex, ex);
+ Master.log.warn("Error closing TabletLocationState iterator: {}",
ex);
{code}
Needs an extra {{ex}} argument passed in to preserve original log statement
(dropping the braces would be better IMO).
{code}
@@ -387,9 +389,9 @@ class TabletGroupWatcher extends Daemon {
return;
}
}
- Master.log.error("Metadata table is inconsistent at " + row + " and all
assigned/future tservers are still online.");
+ Master.log.error("Metadata table is inconsistent at {} and all
assigned/future tservers are still online.", row);
} catch (Throwable e) {
- Master.log.error("Error attempting repair of metadata " + row + ": " +
e, e);
+ Master.log.error("Error attempting repair of metadata {}: {}", row, e);
}
}
@@ -427,15 +429,15 @@ class TabletGroupWatcher extends Daemon {
TServerConnection conn;
conn = this.master.tserverSet.getConnection(tls.current);
if (conn != null) {
- Master.log.info("Asking " + tls.current + " to split " +
tls.extent + " at " + splitPoint);
+ Master.log.info("Asking {} to split {} at {}", tls.current,
tls.extent, splitPoint);
conn.splitTablet(this.master.masterLock, tls.extent, splitPoint);
} else {
- Master.log.warn("Not connected to server " + tls.current);
+ Master.log.warn("Not connected to server {}", tls.current);
}
} catch (NotServingTabletException e) {
- Master.log.debug("Error asking tablet server to split a tablet: " +
e);
+ Master.log.debug("Error asking tablet server to split a tablet: {}",
e);
} catch (Exception e) {
- Master.log.warn("Error asking tablet server to split a tablet: " +
e);
+ Master.log.warn("Error asking tablet server to split a tablet: {}",
e);
{code}
Same as last: need extra {{e}} or drop the braces
{code}
@@ -495,7 +497,7 @@ class TabletGroupWatcher extends Daemon {
}
}
} catch (Exception ex) {
- Master.log.error("Unable to update merge state for merge " +
stats.getMergeInfo().getExtent(), ex);
+ Master.log.error("Unable to update merge state for merge {} {}",
stats.getMergeInfo().getExtent(), ex);
{code}
Extra braces
{code}
@@ -102,9 +102,9 @@ public class RecoveryManager {
initiateSort(sortId, source, destination, aconf);
}
} catch (FileNotFoundException e) {
- log.debug("Unable to initate log sort for " + source + ": " + e);
+ log.debug("Unable to initate log sort for {}: ", source, e);
} catch (Exception e) {
- log.warn("Failed to initiate log sort " + source, e);
+ log.warn("Failed to initiate log sort {} {}", source, e);
{code}
Extra braces
{code}
@@ -67,7 +67,7 @@ public class ReplicationDriver extends Daemon {
conn = master.getConnector();
} catch (AccumuloException | AccumuloSecurityException e) {
// couldn't get a connector, try again in a "short" amount of time
- log.warn("Error trying to get connector to process replication
records", e);
+ log.warn("Error trying to get connector to process replication
records {}", e);
UtilWaitThread.sleep(2000);
continue;
}
@@ -86,21 +86,21 @@ public class ReplicationDriver extends Daemon {
try {
statusMaker.run();
} catch (Exception e) {
- log.error("Caught Exception trying to create Replication status
records", e);
+ log.error("Caught Exception trying to create Replication status
records {}", e);
}
// Tell the work maker to make work
try {
workMaker.run();
} catch (Exception e) {
- log.error("Caught Exception trying to create Replication work
records", e);
+ log.error("Caught Exception trying to create Replication work records
{}", e);
}
// Update the status records from the work records
try {
finishedWorkUpdater.run();
} catch (Exception e) {
- log.error("Caught Exception trying to update Replication records using
finished work records", e);
+ log.error("Caught Exception trying to update Replication records using
finished work records {}", e);
}
// Clean up records we no longer need.
@@ -109,7 +109,7 @@ public class ReplicationDriver extends Daemon {
try {
rcrr.run();
} catch (Exception e) {
- log.error("Caught Exception trying to remove finished Replication
records", e);
+ log.error("Caught Exception trying to remove finished Replication
records {}", e);
}
Trace.off();
@@ -120,7 +120,7 @@ public class ReplicationDriver extends Daemon {
try {
Thread.sleep(sleepMillis);
} catch (InterruptedException e) {
- log.error("Interrupted while sleeping", e);
+ log.error("Interrupted while sleeping {}", e);
}
{code}
Extra braces
{code}
@@ -134,10 +134,10 @@ public class BulkImport extends MasterRepo {
// move the files into the directory
try {
String bulkDir = prepareBulkImport(master, fs, sourceDir, tableId);
- log.debug(" tid " + tid + " bulkDir " + bulkDir);
+ log.debug(" tid {} bulkDir {}", tid, bulkDir);
return new LoadFiles(tableId, sourceDir, bulkDir, errorDir, setTime);
} catch (IOException ex) {
- log.error("error preparing the bulk import directory", ex);
+ log.error("error preparing the bulk import directory {}", ex);
{code}
Extra braces
{code}diff --git
a/server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java
b/server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java
index 0fb9138..209c03a 100644
--- a/server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java
+++ b/server/master/src/main/java/org/apache/accumulo/master/tableOps/Utils.java
@@ -70,7 +70,7 @@ public class Utils {
});
return new String(nid, UTF_8);
} catch (Exception e1) {
- log.error("Failed to assign tableId to " + tableName, e1);
+ LoggerFactory.getLogger(CreateTable.class).error("Failed to assign
tableId to {} {}", tableName, e1);
throw new ThriftTableOperationException(tableId, tableName,
TableOperation.CREATE, TableOperationExceptionType.OTHER, e1.getMessage());
}
}
{code}
Extra braces
{code}
@@ -386,7 +386,7 @@ public class Monitor {
}
}
} catch (Exception ex) {
- log.warn("Unable to contact the garbage collector at " + address, ex);
+ log.warn("Unable to contact the garbage collector at {} {}", address,
ex);
}
return result;
}
@@ -427,10 +427,10 @@ public class Monitor {
Monitor.START_TIME = System.currentTimeMillis();
int port = config.getConfiguration().getPort(Property.MONITOR_PORT);
try {
- log.debug("Creating monitor on port " + port);
+ log.debug("Creating monitor on port {}", port);
server = new EmbeddedWebServer(hostname, port);
} catch (Throwable ex) {
- log.error("Unable to start embedded web server", ex);
+ log.error("Unable to start embedded web server {}", ex);
throw new RuntimeException(ex);
}
{code}
Extra braces
{code}
diff --git
a/server/monitor/src/main/java/org/apache/accumulo/monitor/ZooKeeperStatus.java
b/server/monitor/src/main/java/org/apache/accumulo/monitor/ZooKeeperStatus.java
index 2e89344..af146f6 100644
---
a/server/monitor/src/main/java/org/apache/accumulo/monitor/ZooKeeperStatus.java
+++
b/server/monitor/src/main/java/org/apache/accumulo/monitor/ZooKeeperStatus.java
@@ -129,7 +129,7 @@ public class ZooKeeperStatus implements Runnable {
}
update.add(new ZooKeeperState(keeper, mode, clients));
} catch (Exception ex) {
- log.info("Exception talking to zookeeper " + keeper, ex);
+ log.info("Exception talking to zookeeper {} {}", keeper, ex);
update.add(new ZooKeeperState(keeper, "Down", -1));
} finally {
if (transport != null) {
diff --git
a/server/tracer/src/main/java/org/apache/accumulo/tracer/AsyncSpanReceiver.java
b/server/tracer/src/main/java/org/apache/accumulo/tracer/AsyncSpanReceiver.java
index 0ade243..44e622e 100644
---
a/server/tracer/src/main/java/org/apache/accumulo/tracer/AsyncSpanReceiver.java
+++
b/server/tracer/src/main/java/org/apache/accumulo/tracer/AsyncSpanReceiver.java
@@ -132,7 +132,7 @@ public abstract class
AsyncSpanReceiver<SpanKey,Destination> implements SpanRece
}
sent = true;
} catch (Exception ex) {
- log.warn("Got error sending to " + dest + ", refreshing client", ex);
+ log.warn("Got error sending to {}, refreshing client {}", dest, ex);
clients.remove(dest);
}
}
{code}
Extra braces
{code}
@@ -71,7 +72,24 @@ public class Module extends Node {
String print;
if ((print = props.getProperty("print")) != null) {
Level level = Level.toLevel(print);
- log.log(level, name);
+ switch(level.toInt()) {
+ case Level.TRACE_INT:
+ log.trace(name);
+ break;
+ case Level.DEBUG_INT:
+ log.debug(name);
+ break;
+ case Level.ERROR_INT:
+ case Level.FATAL_INT:
+ log.error(name);
+ break;
+ case Level.INFO_INT:
+ log.info(name);
+ break;
+ case Level.WARN_INT:
+ log.warn(name);
+ }
+ //log.log(level, name);
{code}
Unnecessary comment.
> Remove string concatenation in log statements where slf4j is used.
> ------------------------------------------------------------------
>
> Key: ACCUMULO-3652
> URL: https://issues.apache.org/jira/browse/ACCUMULO-3652
> Project: Accumulo
> Issue Type: Sub-task
> Components: build
> Affects Versions: 1.7.0
> Reporter: Ed Coleman
> Assignee: Ed Coleman
> Priority: Minor
> Fix For: 1.8.0
>
> Attachments: ACCUMULO-3652-3.patch, ACCUMULO-3652-4.patch
>
>
> As a separate task, continue with the conversion to remove log4j
> dependencies, modify log statements where string concatenation is used and
> replace with {} parameter substitution.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
