Address PR comments

yahoo · Oct 31, 2017 · 8a56af3 · 8a56af3
1 parent af29a83
commit 8a56af3
Show file tree

Hide file tree

Showing 5 changed files with 10 additions and 22 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -91,7 +91,7 @@ Current
 
 ### Changed:
 
-- [Added mandatory logging for LoadTask errors]
+- [Added mandatory logging for LoadTask errors](https://github.com/yahoo/fili/pull/553)
     * Finalized `run()` in `LoadTask` with mandatory logging of exceptions.
     * Created mandatory runInner to replace run in all `LoadTask` instances
 

diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/application/LoadTask.java b/fili-core/src/main/java/com/yahoo/bard/webservice/application/LoadTask.java
@@ -4,6 +4,7 @@
 
 import com.yahoo.bard.webservice.druid.client.FailureCallback;
 import com.yahoo.bard.webservice.druid.client.HttpErrorCallback;
+import com.yahoo.bard.webservice.web.ErrorMessageFormat;
 
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -20,7 +21,6 @@
 public abstract class LoadTask<V> implements Runnable {
 
     private static final Logger LOG = LoggerFactory.getLogger(LoadTask.class);
-    public static final String LOAD_TASK_ERROR_FORMAT = "Exception while running %s: %s";
 
     protected final String loaderName;
     protected final long delay;
@@ -65,7 +65,7 @@ public final void run() {
         try {
             runInner();
         } catch (RuntimeException t) {
-            LOG.error(String.format(LOAD_TASK_ERROR_FORMAT, getName(), t.getMessage()), t);
+            LOG.error(ErrorMessageFormat.LOAD_TASK_FAILURE.logFormat(getName(), t.getMessage()), t);
             failed = true;
             throw t;
         }

diff --git a/...ore/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/LuceneSearchProvider.java b/...ore/src/main/java/com/yahoo/bard/webservice/data/dimension/impl/LuceneSearchProvider.java
@@ -458,7 +458,7 @@ public void clearDimension() {
                 writer.deleteAll();
                 writer.commit();
             } catch (IOException e) {
-                LOG.error(ErrorMessageFormat.FAIL_TO_WIPTE_LUCENE_INDEX_DIR.format(luceneDirectory));
+                LOG.error(ErrorMessageFormat.FAIL_TO_WIPE_LUCENE_INDEX_DIR.format(luceneDirectory));
                 throw new RuntimeException(e);
             }
 

diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java b/fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java
@@ -259,7 +259,9 @@ public enum ErrorMessageFormat implements MessageFormatter {
 
     UNABLE_TO_CREATE_DIR("Unable to create directory %s."),
     UNABLE_TO_DELETE_DIR("Unable to delete directory %s."),
-    FAIL_TO_WIPTE_LUCENE_INDEX_DIR("Failed to wipe Lucene index at directory: %s")
+    FAIL_TO_WIPE_LUCENE_INDEX_DIR("Failed to wipe Lucene index at directory: %s"),
+
+    LOAD_TASK_FAILURE("Exception while running %s: %s")
     ;
 
     private final String messageFormat;

diff --git a/fili-core/src/test/groovy/com/yahoo/bard/webservice/application/LoadTaskSpec.groovy b/fili-core/src/test/groovy/com/yahoo/bard/webservice/application/LoadTaskSpec.groovy
@@ -10,8 +10,6 @@ import com.yahoo.bard.webservice.druid.client.FailureCallback
 import com.yahoo.bard.webservice.druid.client.HttpErrorCallback
 import com.yahoo.bard.webservice.logging.TestLogAppender
 
-import com.fasterxml.jackson.databind.ObjectMapper
-
 import spock.lang.Ignore
 import spock.lang.Specification
 import spock.lang.Timeout
@@ -240,8 +238,7 @@ class LoadTaskSpec extends Specification {
 
     class FailingTestLoadTask extends LoadTaskSpec.OneOffTestLoadTask {
 
-
-        public static final String UNIQUE_VALUE_1 = "UNIQUE_VALUE_1"
+        public static final String UNIQUE_VALUE_1 = "FailingTestLoadTask.UNIQUE_VALUE_1"
 
         FailingTestLoadTask(long delay) {
             super(delay)
@@ -253,26 +250,15 @@ class LoadTaskSpec extends Specification {
         }
     }
 
-
     def "Test error gets logged on failed test"() {
-        setup:
         setup: "Instantiate a task scheduler"
-        ObjectMapper MAPPER = new ObjectMappersSuite().getMapper()
-
-
         TaskScheduler scheduler = new TaskScheduler(2)
         LoadTask<Boolean> loader = new FailingTestLoadTask(expectedDelay)
-        loader.setFuture(
-                scheduler.schedule(
-                        loader,
-                        1,
-                        TimeUnit.MILLISECONDS
-                )
-        )
+        loader.setFuture(scheduler.schedule(loader,1, TimeUnit.MILLISECONDS))
         Thread.sleep(10)
 
         expect:
         loader.failed
-        logAppender.getMessages().find { it.contains(/UNIQUE_VALUE_1/) }
+        logAppender.getMessages().find { it.contains(/FailingTestLoadTask.UNIQUE_VALUE_1/) }
     }
 }