further review comments addressed

apache · Dec 13, 2016 · 8293b42 · 8293b42
1 parent e9dae87
commit 8293b42
Show file tree

Hide file tree

Showing 6 changed files with 47 additions and 5 deletions.
diff --git a/...sions-contrib/google-extensions/src/main/java/io/druid/storage/google/GoogleTaskLogs.java b/...sions-contrib/google-extensions/src/main/java/io/druid/storage/google/GoogleTaskLogs.java
@@ -106,4 +106,10 @@ public void killAll() throws IOException
 	{
 		throw new UnsupportedOperationException("not implemented");
 	}
+
+  @Override
+  public void killOlderThan(long timestamp) throws IOException
+  {
+    throw new UnsupportedOperationException("not implemented");
+  }
 }
diff --git a/extensions-core/hdfs-storage/src/main/java/io/druid/storage/hdfs/tasklog/HdfsTaskLogs.java b/extensions-core/hdfs-storage/src/main/java/io/druid/storage/hdfs/tasklog/HdfsTaskLogs.java
@@ -132,6 +132,11 @@ public void killOlderThan(long timestamp) throws IOException
     Path taskLogDir = new Path(config.getDirectory());
     FileSystem fs = taskLogDir.getFileSystem(hadoopConfig);
     if (fs.exists(taskLogDir)) {
+
+      if (!fs.isDirectory(taskLogDir)) {
+        throw new IOException(String.format("taskLogDir [%s] must be a directory.", taskLogDir));
+      }
+
       RemoteIterator<LocatedFileStatus> iter = fs.listLocatedStatus(taskLogDir);
       while (iter.hasNext()) {
         LocatedFileStatus file = iter.next();

diff --git a/indexing-service/src/main/java/io/druid/indexing/common/tasklogs/FileTaskLogs.java b/indexing-service/src/main/java/io/druid/indexing/common/tasklogs/FileTaskLogs.java
@@ -96,6 +96,11 @@ public void killOlderThan(final long timestamp) throws IOException
   {
     File taskLogDir = config.getDirectory();
     if (taskLogDir.exists()) {
+
+      if (!taskLogDir.isDirectory()) {
+        throw new IOException(String.format("taskLogDir [%s] must be a directory.", taskLogDir));
+      }
+
       File[] files = taskLogDir.listFiles(
           new FileFilter()
           {

diff --git a/indexing-service/src/main/java/io/druid/indexing/overlord/helpers/OverlordHelperManager.java b/indexing-service/src/main/java/io/druid/indexing/overlord/helpers/OverlordHelperManager.java
@@ -38,7 +38,7 @@ public class OverlordHelperManager
   private final Set<OverlordHelper> helpers;
 
   private volatile ScheduledExecutorService exec;
-  private final Object lock = new Object();
+  private final Object startStopLock = new Object();
   private volatile boolean started = false;
 
   @Inject
@@ -53,7 +53,7 @@ public OverlordHelperManager(
   @LifecycleStart
   public void start()
   {
-    synchronized (lock) {
+    synchronized (startStopLock) {
       if (!started) {
         log.info("OverlordHelperManager is starting.");
 
@@ -75,7 +75,7 @@ public void start()
   @LifecycleStop
   public void stop()
   {
-    synchronized (lock) {
+    synchronized (startStopLock) {
       if (started) {
         log.info("OverlordHelperManager is stopping.");
         if (exec != null) {

diff --git a/...ng-service/src/main/java/io/druid/indexing/overlord/helpers/TaskLogAutoCleanerConfig.java b/...ng-service/src/main/java/io/druid/indexing/overlord/helpers/TaskLogAutoCleanerConfig.java
@@ -56,11 +56,15 @@ public TaskLogAutoCleanerConfig(
     this.initialDelay = initialDelay == null ? 60000 + new Random().nextInt(4*60000) : initialDelay.longValue();
     this.delay = delay == null ? 6*60*60*1000 : delay.longValue();
     this.durationToRetain = durationToRetain == null ? Long.MAX_VALUE : durationToRetain.longValue();
+
+    Preconditions.checkArgument(this.initialDelay > 0, "initialDelay must be > 0.");
+    Preconditions.checkArgument(this.delay > 0, "delay must be > 0.");
+    Preconditions.checkArgument(this.durationToRetain > 0, "durationToRetain must be > 0.");
   }
 
   public boolean isEnabled()
   {
-    return true;
+    return this.enabled;
   }
 
   public long getInitialDelay()

diff --git a/...ervice/src/test/java/io/druid/indexing/overlord/helpers/TaskLogAutoCleanerConfigTest.java b/...ervice/src/test/java/io/druid/indexing/overlord/helpers/TaskLogAutoCleanerConfigTest.java
@@ -49,9 +49,31 @@ public void testSerde() throws Exception
         ), TaskLogAutoCleanerConfig.class
     );
 
-    Assert.assertEquals(true, config.isEnabled());
+    Assert.assertTrue(config.isEnabled());
     Assert.assertEquals(10, config.getInitialDelay());
     Assert.assertEquals(40, config.getDelay());
     Assert.assertEquals(30, config.getDurationToRetain());
   }
+
+  @Test
+  public void testSerdeWithDefaults() throws Exception
+  {
+    String json = "{}";
+
+    ObjectMapper mapper = TestUtil.MAPPER;
+
+    TaskLogAutoCleanerConfig config = mapper.readValue(
+        mapper.writeValueAsString(
+            mapper.readValue(
+                json,
+                TaskLogAutoCleanerConfig.class
+            )
+        ), TaskLogAutoCleanerConfig.class
+    );
+
+    Assert.assertFalse(config.isEnabled());
+    Assert.assertTrue(config.getInitialDelay() >= 60000 && config.getInitialDelay() <= 300000);
+    Assert.assertEquals(6*60*60*1000, config.getDelay());
+    Assert.assertEquals(Long.MAX_VALUE, config.getDurationToRetain());
+  }
 }