accumulo-notifications mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (ACCUMULO-3652) Remove string concatenation in log statements where slf4j is used.
Date Mon, 04 May 2015 14:59:07 GMT

    [ 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)

Mime
View raw message