commit de10fcd5b3bc9d29c0e2a011728e34fbe9b0fd5d Author: Karsten Loesing <karsten.loesing@gmx.net> Date: Wed Nov 20 13:03:25 2019 +0100 Stop checking and reloading configuration file. Removes a deprecation warning and simplifies code. Implements #32554. --- CHANGELOG.md | 4 ++ .../org/torproject/metrics/collector/Main.java | 2 +- .../metrics/collector/conf/Configuration.java | 59 +++------------------- .../metrics/collector/cron/CollecTorMain.java | 28 +--------- src/main/resources/collector.properties | 9 ---- .../org/torproject/metrics/collector/MainTest.java | 6 +-- .../metrics/collector/conf/ConfigurationTest.java | 58 --------------------- 7 files changed, 16 insertions(+), 150 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5293207..b55d7a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,9 @@ # Changes in version 1.1?.? - 2019-1?-?? + * Medium changes + - Give up on periodically checking the configuration file for + updates and reloading it in case of changes. + # Changes in version 1.13.1 - 2019-11-11 diff --git a/src/main/java/org/torproject/metrics/collector/Main.java b/src/main/java/org/torproject/metrics/collector/Main.java index 3438bda..861567f 100644 --- a/src/main/java/org/torproject/metrics/collector/Main.java +++ b/src/main/java/org/torproject/metrics/collector/Main.java @@ -85,7 +85,7 @@ public class Main { writeDefaultConfig(confPath); return; } else { - conf.setWatchableSourceAndLoad(confPath); + conf.loadAndCheckConfiguration(confPath); } Scheduler.getInstance().scheduleModuleRuns(collecTorMains, conf); } catch (ConfigurationException ce) { diff --git a/src/main/java/org/torproject/metrics/collector/conf/Configuration.java b/src/main/java/org/torproject/metrics/collector/conf/Configuration.java index 4bb581b..cfdd9c8 100644 --- a/src/main/java/org/torproject/metrics/collector/conf/Configuration.java +++ b/src/main/java/org/torproject/metrics/collector/conf/Configuration.java @@ -3,85 +3,40 @@ package org.torproject.metrics.collector.conf; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - import java.io.FileInputStream; import java.io.IOException; import java.io.InputStream; import java.net.MalformedURLException; import java.net.URL; -import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; -import java.nio.file.attribute.FileTime; import java.util.EnumSet; -import java.util.Observable; import java.util.Properties; import java.util.Set; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; /** * Initialize configuration with defaults from collector.properties, * unless a configuration properties file is available. */ -public class Configuration extends Observable implements Cloneable { - - private static final Logger logger = LoggerFactory.getLogger( - Configuration.class); - - private final ScheduledExecutorService scheduler = - Executors.newScheduledThreadPool(1); +public class Configuration { public static final String FIELDSEP = ","; private final Properties props = new Properties(); - private Path configurationFile; - private FileTime ft; /** - * Load the configuration from the given path and start monitoring changes. - * If the file was changed, re-read and inform all observers. + * Load the configuration from the given path. */ - public void setWatchableSourceAndLoad(final Path confPath) throws + public void loadAndCheckConfiguration(Path confPath) throws ConfigurationException { - this.configurationFile = confPath; - try { - ft = Files.getLastModifiedTime(confPath); - reload(); + try (FileInputStream fis + = new FileInputStream(confPath.toFile())) { + this.props.load(fis); anythingActivated(); } catch (IOException e) { - throw new ConfigurationException("Cannot watch configuration file. " + throw new ConfigurationException("Cannot load configuration file. " + "Reason: " + e.getMessage(), e); } - if (this.getBool(Key.RunOnce)) { // no need to watch - return; - } - this.scheduler.scheduleAtFixedRate(() -> { - logger.trace("Check configuration file."); - try { - FileTime ftNow = Files.getLastModifiedTime(confPath); - if (ft.compareTo(ftNow) < 0) { - logger.info("Configuration file was changed."); - reload(); - setChanged(); - notifyObservers(null); - } - ft = ftNow; - } catch (Throwable th) { // Catch all and keep running. - logger.error("Cannot reload configuration file.", th); - } - }, 5, 5, TimeUnit.SECONDS); - } - - private void reload() throws IOException { - props.clear(); - try (FileInputStream fis - = new FileInputStream(configurationFile.toFile())) { - props.load(fis); - } } private void anythingActivated() throws ConfigurationException { diff --git a/src/main/java/org/torproject/metrics/collector/cron/CollecTorMain.java b/src/main/java/org/torproject/metrics/collector/cron/CollecTorMain.java index cd8e0ee..09796e3 100644 --- a/src/main/java/org/torproject/metrics/collector/cron/CollecTorMain.java +++ b/src/main/java/org/torproject/metrics/collector/cron/CollecTorMain.java @@ -19,31 +19,24 @@ import java.nio.file.Path; import java.util.Collections; import java.util.HashMap; import java.util.Map; -import java.util.Observable; -import java.util.Observer; import java.util.concurrent.Callable; -import java.util.concurrent.atomic.AtomicBoolean; public abstract class CollecTorMain extends SyncManager - implements Callable<Object>, Observer, Runnable { + implements Callable<Object>, Runnable { private static final Logger logger = LoggerFactory.getLogger( CollecTorMain.class); private static final long LIMIT_MB = 200; public static final String SOURCES = "Sources"; - private final AtomicBoolean newConfigAvailable = new AtomicBoolean(false); protected Configuration config = new Configuration(); - private Configuration newConfig; - protected final Map<String, Class<? extends Descriptor>> mapPathDescriptors = new HashMap<>(); public CollecTorMain(Configuration conf) { this.config.putAll(conf.getPropertiesCopy()); - conf.addObserver(this); } /** @@ -51,16 +44,6 @@ public abstract class CollecTorMain extends SyncManager */ @Override public final void run() { - synchronized (this) { - if (newConfigAvailable.get()) { - logger.info("Module {} is using the new configuration.", module()); - synchronized (newConfig) { - config.clear(); - config.putAll(newConfig.getPropertiesCopy()); - newConfigAvailable.set(false); - } - } - } try { if (!isSyncOnly()) { logger.info("Starting {} module of CollecTor.", module()); @@ -104,15 +87,6 @@ public abstract class CollecTorMain extends SyncManager return null; } - @Override - public synchronized void update(Observable obs, Object obj) { - newConfigAvailable.set(true); - if (obs instanceof Configuration) { - newConfig = (Configuration) obs; - logger.info("Module {} just received a new configuration.", module()); - } - } - /** * Module specific code goes here. */ diff --git a/src/main/resources/collector.properties b/src/main/resources/collector.properties index 65e0f99..6422120 100644 --- a/src/main/resources/collector.properties +++ b/src/main/resources/collector.properties @@ -1,9 +1,6 @@ ######## Collector Properties # ######## Run Configuration ######## -## This part of the configuration cannot be updated at runtime! -## Changes require a restart in order to become effective. -## # If RunOnce=true, the activated modules below will only be # run one time and without any delay. # Make sure only to run non-interfering modules together. @@ -66,12 +63,6 @@ BridgedbMetricsPeriodMinutes = 480 # offset in minutes since the epoch and BridgedbMetricsOffsetMinutes = 340 -########################################## -## 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 ######## # The URL of this instance. This will be the base URL # written to index.json, i.e. please change this to the mirrors url! diff --git a/src/test/java/org/torproject/metrics/collector/MainTest.java b/src/test/java/org/torproject/metrics/collector/MainTest.java index 820cfb6..f3f2500 100644 --- a/src/test/java/org/torproject/metrics/collector/MainTest.java +++ b/src/test/java/org/torproject/metrics/collector/MainTest.java @@ -43,10 +43,10 @@ public class MainTest { Configuration conf = new Configuration(); thrown.expect(ConfigurationException.class); thrown.expectMessage(Matchers - .containsString("Cannot watch configuration file.")); + .containsString("Cannot load configuration file.")); // dir instead of file; the following should throw a ConfigurationException - conf.setWatchableSourceAndLoad(tmpFolder.toPath()); + conf.loadAndCheckConfiguration(tmpFolder.toPath()); } private void checkCleanEnv(File conf) { @@ -210,7 +210,7 @@ public class MainTest { thrown.expectMessage(Matchers.containsString("Nothing is activated!")); // no module activated; the following should throw a ConfigurationException - conf.setWatchableSourceAndLoad(confPath); + conf.loadAndCheckConfiguration(confPath); } } diff --git a/src/test/java/org/torproject/metrics/collector/conf/ConfigurationTest.java b/src/test/java/org/torproject/metrics/collector/conf/ConfigurationTest.java index e01aba2..6f5d16c 100644 --- a/src/test/java/org/torproject/metrics/collector/conf/ConfigurationTest.java +++ b/src/test/java/org/torproject/metrics/collector/conf/ConfigurationTest.java @@ -8,25 +8,16 @@ import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import org.torproject.metrics.collector.MainTest; -import org.torproject.metrics.collector.cron.CollecTorMain; -import org.torproject.metrics.collector.cron.Dummy; - -import org.junit.Ignore; import org.junit.Rule; import org.junit.Test; import org.junit.rules.TemporaryFolder; import java.io.ByteArrayInputStream; import java.io.File; -import java.lang.reflect.Field; import java.net.URL; -import java.nio.file.Files; -import java.nio.file.Paths; import java.util.Arrays; import java.util.Random; import java.util.Set; -import java.util.concurrent.atomic.AtomicBoolean; public class ConfigurationTest { @@ -167,53 +158,4 @@ public class ConfigurationTest { conf.setProperty(Key.BridgeDescriptorMappingsLimit.name(), "y7"); conf.getInt(Key.BridgeDescriptorMappingsLimit); } - - @Test(expected = ConfigurationException.class) - public void testSetWatchableSourceAndLoad() throws Exception { - Configuration conf = new Configuration(); - conf.setWatchableSourceAndLoad(Paths.get("/tmp/phantom.path")); - } - - @Ignore("This test takes 13 seconds, which is too long.") - @Test() - public void testConfigChange() throws Exception { - Configuration conf = new Configuration(); - final AtomicBoolean called = new AtomicBoolean(false); - conf.addObserver((obs, obj) -> called.set(true)); - File confFile = tmpf.newFile("empty"); - Files.write(confFile.toPath(), (Key.RelaydescsActivated.name() + "=true") - .getBytes()); - conf.setWatchableSourceAndLoad(confFile.toPath()); - MainTest.waitSec(1); - confFile.setLastModified(System.currentTimeMillis()); - MainTest.waitSec(6); - assertTrue("Update was not called.", called.get()); - called.set(false); - 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((obs, obj) -> called.set(true)); - File confFile = tmpf.newFile("empty"); - Files.write(confFile.toPath(), (Key.RelaydescsActivated.name() + "=true") - .getBytes()); - conf.setWatchableSourceAndLoad(confFile.toPath()); - MainTest.waitSec(1); - confFile.delete(); - conf.setProperty(Key.RunOnce.name(), "false"); - final Dummy dummy = new Dummy(conf); - tmpf.newFolder("empty"); - 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); - } - }