[tor-commits] [collector/master] Implements changes discussed in #19720 comment 9 and below.

[karsten at torproject.org]

commit b8361e7d4fe447ba01306bb59ad9c213706d10b6
Author: iwakeh <iwakeh at torproject.org>
Date:   Tue Aug 9 17:03:24 2016 +0200

    Implements changes discussed in #19720 comment 9 and below.
    
    Reduce check-interval, added test for wrong configuration,
    added comment to collector.properties, introduced catch-all,
    added log-statements.
---
 .../torproject/collector/conf/Configuration.java   |  6 ++--
 .../torproject/collector/cron/CollecTorMain.java   |  3 +-
 src/main/resources/collector.properties            |  4 +++
 .../collector/conf/ConfigurationTest.java          | 32 +++++++++++++++++++---
 4 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/src/main/java/org/torproject/collector/conf/Configuration.java b/src/main/java/org/torproject/collector/conf/Configuration.java
index 205a977..0772bd5 100644
--- a/src/main/java/org/torproject/collector/conf/Configuration.java
+++ b/src/main/java/org/torproject/collector/conf/Configuration.java
@@ -70,11 +70,11 @@ public class Configuration extends Observable implements Cloneable {
                 notifyObservers(null);
               }
               ft = ftNow;
-            } catch (IOException | RuntimeException re) {
-              log.error("Cannot reload configuration file.", re);
+            } catch (Throwable th) { // Catch all and keep running.
+              log.error("Cannot reload configuration file.", th);
             }
         }
-      }, 1, 1, TimeUnit.MINUTES);
+      }, 5, 5, TimeUnit.SECONDS);
   }
 
   private final void reload() throws IOException {
diff --git a/src/main/java/org/torproject/collector/cron/CollecTorMain.java b/src/main/java/org/torproject/collector/cron/CollecTorMain.java
index 34d711a..fa33fd3 100644
--- a/src/main/java/org/torproject/collector/cron/CollecTorMain.java
+++ b/src/main/java/org/torproject/collector/cron/CollecTorMain.java
@@ -47,7 +47,7 @@ public abstract class CollecTorMain implements Observer, Runnable {
   public final void run() {
     synchronized (this) {
       if (newConfigAvailable.get()) {
-        log.info("Module {} received new configuration.", module());
+        log.info("Module {} is using the new configuration.", module());
         synchronized (newConfig) {
           config.clear();
           config.putAll(newConfig.getPropertiesCopy());
@@ -69,6 +69,7 @@ public abstract class CollecTorMain implements Observer, Runnable {
     newConfigAvailable.set(true);
     if (obs instanceof Configuration) {
       newConfig = (Configuration) obs;
+      log.info("Module {} just received a new configuration.", module());
     }
   }
 
diff --git a/src/main/resources/collector.properties b/src/main/resources/collector.properties
index 0999fcb..4469052 100644
--- a/src/main/resources/collector.properties
+++ b/src/main/resources/collector.properties
@@ -40,6 +40,10 @@ UpdateindexPeriodMinutes = 2
 UpdateindexOffsetMinutes = 0
 ##########################################
 ## All below can be changed at runtime.
+#####
+## Every five secands the running CollecTor checks this file for changes.
+## When the file was modified, all activated modules are informed and will
+## use the new configuration in their next scheduled run.
 ######## General Properties ########
 InstanceBaseUrl = "https://collector.torproject.org"
 LockFilePath = lock
diff --git a/src/test/java/org/torproject/collector/conf/ConfigurationTest.java b/src/test/java/org/torproject/collector/conf/ConfigurationTest.java
index 14033dc..c98366d 100644
--- a/src/test/java/org/torproject/collector/conf/ConfigurationTest.java
+++ b/src/test/java/org/torproject/collector/conf/ConfigurationTest.java
@@ -8,6 +8,8 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import org.torproject.collector.MainTest;
+import org.torproject.collector.cron.CollecTorMain;
+import org.torproject.collector.cron.Dummy;
 
 import org.junit.Rule;
 import org.junit.Test;
@@ -15,6 +17,7 @@ import org.junit.rules.TemporaryFolder;
 
 import java.io.ByteArrayInputStream;
 import java.io.File;
+import java.lang.reflect.Field;
 import java.nio.file.Paths;
 import java.util.Arrays;
 import java.util.Observable;
@@ -170,7 +173,7 @@ public class ConfigurationTest {
     conf.setWatchableSourceAndLoad(Paths.get("/tmp/phantom.path"));
   }
 
-    @Test()
+  @Test()
   public void testConfigChange() throws Exception {
     Configuration conf = new Configuration();
     final AtomicBoolean called = new AtomicBoolean(false);
@@ -182,15 +185,36 @@ public class ConfigurationTest {
     File confFile = tmpf.newFile("empty");
     conf.setWatchableSourceAndLoad(confFile.toPath());
     confFile.setLastModified(System.currentTimeMillis());
-    MainTest.waitSec(90);
+    MainTest.waitSec(6);
     assertTrue("Update was not called.", called.get());
     called.set(false);
-    MainTest.waitSec(60);
+    MainTest.waitSec(6);
     assertFalse("Update was called.", called.get());
+  }
+
+  @Test()
+  public void testConfigUnreadable() throws Exception {
+    Configuration conf = new Configuration();
+    final AtomicBoolean called = new AtomicBoolean(false);
+    conf.addObserver(new Observer() {
+        public void update(Observable obs, Object obj) {
+          called.set(true);
+        }
+      });
+    File confFile = tmpf.newFile("empty");
+    conf.setWatchableSourceAndLoad(confFile.toPath());
     confFile.delete();
+    conf.setProperty(Key.CompressRelayDescriptorDownloads.name(), "false");
+    conf.setProperty(Key.ImportDirectoryArchives.name(), "false");
+    Dummy dummy = new Dummy(conf);
     tmpf.newFolder("empty");
-    MainTest.waitSec(60);
+    MainTest.waitSec(6);
+    assertFalse("Update was called.", called.get());
     assertEquals(0, conf.size());
+    Field confField = CollecTorMain.class.getDeclaredField("config");
+    confField.setAccessible(true);
+    int size = ((Configuration)(confField.get(dummy))).size();
+    assertEquals(2, size);
   }
 
 }