[tor-commits] [metrics-lib/master] Added slf4j for logging support; implements #19643.

karsten at torproject.org karsten at torproject.org
Mon Aug 22 09:27:44 UTC 2016


commit 1dab40d4505e731faccd5f722e5f7c250a2d4955
Author: iwakeh <iwakeh at torproject.org>
Date:   Fri Aug 19 13:23:07 2016 +0200

    Added slf4j for logging support; implements #19643.
    
    Tests use logback; runtime will use whatever slf4j implementation supplied.
    Also removed TODOs about testing, b/c this is noticeable in our coverage report.
---
 build.xml                                          |  3 +++
 .../descriptor/impl/DescriptorCollectorImpl.java   | 12 +++++++++---
 .../descriptor/impl/DescriptorReaderImpl.java      | 22 ++++++++++------------
 .../impl/DirectoryKeyCertificateImpl.java          |  2 --
 .../descriptor/impl/DownloadCoordinatorImpl.java   |  9 +++++++--
 .../descriptor/impl/MicrodescriptorImpl.java       |  1 -
 .../descriptor/impl/NetworkStatusEntryImpl.java    |  1 -
 .../descriptor/impl/ServerDescriptorImpl.java      |  1 -
 8 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/build.xml b/build.xml
index bb48dde..bfede7c 100644
--- a/build.xml
+++ b/build.xml
@@ -26,6 +26,7 @@
 
   <patternset id="runtime" >
       <include name="commons-compress-1.9.jar"/>
+      <include name="slf4j-api-1.7.7.jar" />
       <include name="xz-1.5.jar"/>
   </patternset>
 
@@ -52,6 +53,8 @@
     <fileset dir="${libs}">
       <include name="junit4-4.11.jar"/>
       <include name="hamcrest-core-1.3.jar"/>
+      <include name="logback-core-1.1.2.jar" />
+      <include name="logback-classic-1.1.2.jar" />
     </fileset>
   </path>
 
diff --git a/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java b/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java
index 1b35d5f..f9bd60d 100644
--- a/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/DescriptorCollectorImpl.java
@@ -5,6 +5,9 @@ package org.torproject.descriptor.impl;
 
 import org.torproject.descriptor.DescriptorCollector;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.io.BufferedInputStream;
 import java.io.BufferedOutputStream;
 import java.io.BufferedReader;
@@ -31,6 +34,9 @@ import java.util.zip.GZIPInputStream;
 
 public class DescriptorCollectorImpl implements DescriptorCollector {
 
+  private static Logger log = LoggerFactory
+      .getLogger(DescriptorCollectorImpl.class);
+
   @Override
   public void collectDescriptors(String collecTorBaseUrl,
       String[] remoteDirectories, long minLastModified,
@@ -130,7 +136,7 @@ public class DescriptorCollectorImpl implements DescriptorCollector {
         br.close();
       }
     } catch (IOException e) {
-      e.printStackTrace();
+      log.error("Cannot fetch remote directory.", e);
       if (huc != null) {
         huc.disconnect();
       }
@@ -163,7 +169,7 @@ public class DescriptorCollectorImpl implements DescriptorCollector {
       }
       scanner.close();
     } catch (ParseException e) {
-      e.printStackTrace();
+      log.error("Cannot parse directory listing: " + directoryListing, e);
       return null;
     }
     return remoteFiles;
@@ -225,7 +231,7 @@ public class DescriptorCollectorImpl implements DescriptorCollector {
         destinationFile.setLastModified(lastModifiedMillis);
       }
     } catch (IOException e) {
-      e.printStackTrace();
+      log.error("Cannot fetch remote file.", e);
       if (huc != null) {
         huc.disconnect();
       }
diff --git a/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java b/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java
index e3c07e5..fac9475 100644
--- a/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/DescriptorReaderImpl.java
@@ -14,6 +14,9 @@ import org.apache.commons.compress.archivers.tar.TarArchiveInputStream;
 import org.apache.commons.compress.compressors.bzip2.BZip2CompressorInputStream;
 import org.apache.commons.compress.compressors.xz.XZCompressorInputStream;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.io.BufferedInputStream;
 import java.io.BufferedReader;
 import java.io.BufferedWriter;
@@ -34,6 +37,7 @@ import java.util.TreeMap;
 
 public class DescriptorReaderImpl implements DescriptorReader {
 
+  private static Logger log = LoggerFactory.getLogger(DescriptorReaderImpl.class);
   private boolean hasStartedReading = false;
 
   private List<File> directories = new ArrayList<>();
@@ -185,12 +189,8 @@ public class DescriptorReaderImpl implements DescriptorReader {
         this.readTarballs();
         this.hasFinishedReading = true;
       } catch (Throwable t) {
-        /* We're usually not writing to stdout or stderr, but we shouldn't
-         * stay quiet about this potential bug.  If we were to switch to a
-         * logging API, this would qualify as ERROR. */
-        System.err.println("Bug: uncaught exception or error while "
-            + "reading descriptors:");
-        t.printStackTrace();
+        log.error("Bug: uncaught exception or error while "
+            + "reading descriptors: " + t.getMessage(), t);
       } finally {
         this.descriptorQueue.setOutOfDescriptors();
       }
@@ -209,7 +209,7 @@ public class DescriptorReaderImpl implements DescriptorReader {
         String line;
         while ((line = br.readLine()) != null) {
           if (!line.contains(" ")) {
-            /* TODO Handle this problem? */
+            log.warn("Unexpected line structure in old history: " + line);
             continue;
           }
           long lastModifiedMillis = Long.parseLong(line.substring(0,
@@ -218,10 +218,8 @@ public class DescriptorReaderImpl implements DescriptorReader {
           this.excludedFilesBefore.put(absolutePath, lastModifiedMillis);
         }
         br.close();
-      } catch (IOException e) {
-        /* TODO Handle this exception. */
-      } catch (NumberFormatException e) {
-        /* TODO Handle this exception. */
+      } catch (IOException | NumberFormatException e) {
+        log.warn("Trouble reading old history.", e);
       }
     }
 
@@ -246,7 +244,7 @@ public class DescriptorReaderImpl implements DescriptorReader {
         }
         bw.close();
       } catch (IOException e) {
-        /* TODO Handle this exception. */
+        log.warn("Trouble writing new history.", e);
       }
     }
 
diff --git a/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java b/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java
index 21a992d..443bd8b 100644
--- a/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/DirectoryKeyCertificateImpl.java
@@ -18,8 +18,6 @@ import java.util.Set;
 
 import javax.xml.bind.DatatypeConverter;
 
-/* TODO Add test class. */
-
 public class DirectoryKeyCertificateImpl extends DescriptorImpl
     implements DirectoryKeyCertificate {
 
diff --git a/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java b/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java
index 23826aa..05e3936 100644
--- a/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/DownloadCoordinatorImpl.java
@@ -8,6 +8,9 @@ import org.torproject.descriptor.DescriptorRequest;
 import org.torproject.descriptor.DirSourceEntry;
 import org.torproject.descriptor.RelayNetworkStatusConsensus;
 
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -20,6 +23,9 @@ import java.util.TreeSet;
 /* TODO This whole download logic is a mess and needs a cleanup. */
 public class DownloadCoordinatorImpl implements DownloadCoordinator {
 
+  private static Logger log = LoggerFactory
+      .getLogger(DownloadCoordinatorImpl.class);
+
   private BlockingIteratorImpl<DescriptorRequest> descriptorQueue =
       new BlockingIteratorImpl<>();
 
@@ -71,8 +77,7 @@ public class DownloadCoordinatorImpl implements DownloadCoordinator {
     if (this.directoryMirrors.isEmpty()
         && this.directoryAuthorities.isEmpty()) {
       this.descriptorQueue.setOutOfDescriptors();
-      /* TODO Should we say anything if we don't have any directories
-       * configured? */
+      log.warn("There were no directories configured. Nothing to download.");
     } else {
       GlobalTimer globalTimer = new GlobalTimer(this.globalTimeoutMillis,
           this);
diff --git a/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java b/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java
index ab859a0..ecad5cb 100644
--- a/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/MicrodescriptorImpl.java
@@ -147,7 +147,6 @@ public class MicrodescriptorImpl extends DescriptorImpl
           + "'" + line + "'.");
     }
     /* TODO Add more checks. */
-    /* TODO Add tests. */
     this.orAddresses.add(parts[1]);
   }
 
diff --git a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
index 807dd76..f528033 100644
--- a/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/NetworkStatusEntryImpl.java
@@ -154,7 +154,6 @@ public class NetworkStatusEntryImpl implements NetworkStatusEntry {
           + "status entry.");
     }
     /* TODO Add more checks. */
-    /* TODO Add tests. */
     this.orAddresses.add(parts[1]);
   }
 
diff --git a/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java b/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java
index 08b9271..622e9a4 100644
--- a/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java
+++ b/src/main/java/org/torproject/descriptor/impl/ServerDescriptorImpl.java
@@ -260,7 +260,6 @@ public abstract class ServerDescriptorImpl extends DescriptorImpl
           + "'" + line + "'.");
     }
     /* TODO Add more checks. */
-    /* TODO Add tests. */
     this.orAddresses.add(partsNoOpt[1]);
   }
 





More information about the tor-commits mailing list