Assorted cleanup

CHANGELOG.md

@@ -109,6 +109,12 @@ Current
         starts the timer. Note: This won't work when performing timings across threads, or across
         contexts. Those need to be started and stopped manually.
 
+- [Clean up logging and responses in `DimensionCacheLoaderServlet`](https://github.com/yahoo/fili/pull/163)
+    * Switched a number of `error`-level logs to `debug` level to line up with logging guidance when request failures
+      were result of client error
+    * Reduced some `info`-level logs down to `debug`
+    * Converted to 404 when error was cause by not finding a path element
+
 ### Deprecated:
 
 - [CompositePhsyicalTable Core Components Refactor](https://github.com/yahoo/fili/pull/179)

fili-core/src/main/java/com/yahoo/bard/webservice/application/Loader.java

@@ -13,7 +13,7 @@
 import java.util.concurrent.ScheduledFuture;
 
 /**
- * Defines a task that loads data from Druid and is scheduled to run at a given time, potentially periodically.
+ * Defines a task that is scheduled to run at a given time, potentially periodically.
  *
  * @param <V>  The type of the result returned by the task associated with this loader.
  */

fili-core/src/main/java/com/yahoo/bard/webservice/druid/client/DruidClientConfigHelper.java

@@ -9,6 +9,8 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import java.util.concurrent.TimeUnit;
+
 /**
  * Helper to fetch druid url and timeout settings.
  */
@@ -62,7 +64,7 @@ public class DruidClientConfigHelper {
     /**
      * The default timeout for queries.
      */
-    private static final int DRUID_REQUEST_TIMEOUT_DEFAULT = 10 * 60 * 1000; // 10 Minutes, in milliseconds
+    private static final int DRUID_REQUEST_TIMEOUT_DEFAULT = Math.toIntExact(TimeUnit.MINUTES.toMillis(10));
 
     /**
      * Fetches the druid UI request Priority.

fili-core/src/main/java/com/yahoo/bard/webservice/druid/model/DefaultQueryType.java

@@ -5,7 +5,7 @@
 import com.yahoo.bard.webservice.util.EnumUtils;
 
 /**
- * Druid queries that Fili supports out o the box.
+ * Druid queries that Fili supports out of the box.
  */
 public enum DefaultQueryType implements QueryType {
     GROUP_BY,

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/endpoints/DimensionCacheLoaderServletSpec.groovy

@@ -1,6 +1,7 @@
 // Copyright 2016 Yahoo Inc.
 // Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
 package com.yahoo.bard.webservice.web.endpoints
+
 import com.yahoo.bard.webservice.application.JerseyTestBinder
 import com.yahoo.bard.webservice.application.ObjectMappersSuite
 import com.yahoo.bard.webservice.data.cache.DataCache
@@ -27,7 +28,7 @@ import javax.ws.rs.core.Response.Status
 class DimensionCacheLoaderServletSpec extends Specification {
     private static final ObjectMappersSuite MAPPERS = new ObjectMappersSuite()
 
-    static final DateTimeZone ORIGINAL_TIME_ZONE = DateTimeZone.getDefault()
+    DateTimeZone originalTimeZone
 
     DimensionCacheLoaderServlet dimensionCacheLoaderServlet
 
@@ -47,17 +48,16 @@ class DimensionCacheLoaderServletSpec extends Specification {
     DateTime lastUpdated = new DateTime(50000)
 
     def setup() {
+        originalTimeZone = DateTimeZone.getDefault()
         DateTimeZone.setDefault(DateTimeZone.forTimeZone(TimeZone.getTimeZone("America/Chicago")))
 
-        dimensionGenderFields = new LinkedHashSet<>()
-        dimensionGenderFields.add(BardDimensionField.ID)
-        dimensionGenderFields.add(BardDimensionField.DESC)
-
-        dimensionUserCountryFields = new LinkedHashSet<>()
-        dimensionUserCountryFields.add(BardDimensionField.ID)
-        dimensionUserCountryFields.add(BardDimensionField.DESC)
-        dimensionUserCountryFields.add(BardDimensionField.FIELD1)
-        dimensionUserCountryFields.add(BardDimensionField.FIELD2)
+        dimensionGenderFields = [BardDimensionField.ID, BardDimensionField.DESC] as LinkedHashSet<DimensionField>
+        dimensionUserCountryFields = [
+                BardDimensionField.ID,
+                BardDimensionField.DESC,
+                BardDimensionField.FIELD1,
+                BardDimensionField.FIELD2
+        ] as LinkedHashSet<DimensionField>
 
         Set dimensions = [] as Set
 
@@ -101,47 +101,38 @@ class DimensionCacheLoaderServletSpec extends Specification {
     }
 
     def cleanup() {
-        DateTimeZone.setDefault(ORIGINAL_TIME_ZONE)
+        DateTimeZone.setDefault(originalTimeZone)
         dimensionGender.searchProvider.clearDimension()
         dimensionUserCountry.searchProvider.clearDimension()
     }
 
-    def "Check updateDimensionLastUpdated for dimension cache loader endpoint"() {
-
+    def "UpdateDimensionLastUpdated works as expected"() {
         setup:
         DateTime newLastUpdated = new DateTime(10000)
-
         String post = """{"name": "gender","lastUpdated":"$newLastUpdated"}"""
 
-        def dims = dimensionCacheLoaderServlet.dimensionDictionary.findAll()
+        Set<Dimension> dims = dimensionCacheLoaderServlet.dimensionDictionary.findAll()
 
         when:
         Response r = dimensionCacheLoaderServlet.updateDimensionLastUpdated("gender", post)
 
-        then: "The dimension row we updated has changed but the others have not"
-        dims.find { it.apiName == "user_country" }.lastUpdated == lastUpdated
+        then: "The dimension row we updated has changed"
         dims.find { it.apiName == "gender" }.lastUpdated == newLastUpdated
         r.getStatusInfo() == Status.OK
+
+        and: "Others have not"
+        dims.find { it.apiName == "user_country" }.lastUpdated == lastUpdated
     }
 
-    def "Check updateDimensionLastUpdated for missing dimension fails"() {
-
+    def "UpdateDimensionLastUpdated with unknown dimension gives a NOT FOUND response"() {
         setup:
-        DateTime newLastUpdated = new DateTime(10000)
-
-        String post = """{"name": "unknown","lastUpdated":"$newLastUpdated"}"""
-
-        def dims = dimensionCacheLoaderServlet.dimensionDictionary.findAll()
-
-        when:
-        Response r = dimensionCacheLoaderServlet.updateDimensionLastUpdated("unknown", post)
+        String post = """{"name": "unknown","lastUpdated":"${new DateTime(10000)}"}"""
 
-        then: "Bad Request"
-        r.getStatusInfo() == Status.BAD_REQUEST
+        expect:
+        dimensionCacheLoaderServlet.updateDimensionLastUpdated("unknown", post).getStatusInfo() == Status.NOT_FOUND
     }
 
     def "Check addReplaceDimensionRows for dimension cache loader endpoint"() {
-
         setup:
         String post = """{
                            "dimensionRows": [
@@ -192,8 +183,8 @@ class DimensionCacheLoaderServletSpec extends Specification {
 
         then:
         r.getStatusInfo() == Status.OK
-        expectedDimensionUserCountry.searchProvider.findAllDimensionRows().
-                containsAll(dimensionUserCountry.searchProvider.findAllDimensionRows())
+        expectedDimensionUserCountry.searchProvider.findAllDimensionRows()
+                .containsAll(dimensionUserCountry.searchProvider.findAllDimensionRows())
     }
 
     def "Check addUpdateDimensionRows for dimension cache loader endpoint"() {
@@ -251,16 +242,18 @@ class DimensionCacheLoaderServletSpec extends Specification {
 
         then:
         r.getStatusInfo() == Status.OK
-        expectedDimensionUserCountry.searchProvider.findAllDimensionRows().
-                containsAll(dimensionUserCountry.searchProvider.findAllDimensionRows())
+        expectedDimensionUserCountry.searchProvider.findAllDimensionRows()
+                .containsAll(dimensionUserCountry.searchProvider.findAllDimensionRows())
     }
 
-    def "Check servet getDimensionLastUpdated"() {
+    def "Check servlet getDimensionLastUpdated"() {
         setup:
         String expected = """{"name":"gender","lastUpdated":"$lastUpdated"}"""
 
-        expect:
+        when:
         Response r = dimensionCacheLoaderServlet.getDimensionLastUpdated("gender")
+
+        then:
         r.getStatusInfo() == Status.OK
         GroovyTestUtils.compareJson(expected, r.getEntity())
     }
@@ -269,14 +262,16 @@ class DimensionCacheLoaderServletSpec extends Specification {
         setup:
         String expected = """{"cacheStatus":"Active"}"""
 
-        expect:
+        when:
         Response r = dimensionCacheLoaderServlet.getMsg()
+
+        then:
         r.getStatusInfo() == Status.OK
         GroovyTestUtils.compareJson(expected, r.getEntity())
     }
 
     def "Delete data cache"() {
-        // mock new servlet that enforces one call to dataCache clear
+        // mock new servlet that validates one call to dataCache clear
         DataCache dataCache2 = Mock(DataCache)
         1 * dataCache2.clear()
         def dimensionCacheLoaderServlet2 = new DimensionCacheLoaderServlet(
@@ -285,28 +280,19 @@ class DimensionCacheLoaderServletSpec extends Specification {
                 MAPPERS
         )
 
-        when:
-        Response r = dimensionCacheLoaderServlet2.deleteDataCache()
-
-        then:
-        r.getStatusInfo() == Status.OK
+        expect:
+        dimensionCacheLoaderServlet2.deleteDataCache().getStatusInfo() == Status.OK
     }
 
     def "Delete data cache endpoint"() {
         setup:
         JerseyTestBinder jtb = new JerseyTestBinder(DimensionCacheLoaderServlet.class)
 
-        when: "DELETE"
-        Response r = jtb.getHarness().target("cache/data").request().delete()
-
-        then: "OK"
-        Status.OK.getStatusCode() == r.getStatus()
-
-        when: "GET"
-        r = jtb.getHarness().target("cache/data").request().get()
+        expect: "DELETE is allowed"
+        jtb.getHarness().target("cache/data").request().delete().getStatusInfo() == Status.OK
 
-        then: "method not allowed"
-        Status.METHOD_NOT_ALLOWED.getStatusCode() == r.getStatus()
+        and: "GET is not"
+        jtb.getHarness().target("cache/data").request().get().getStatusInfo() == Status.METHOD_NOT_ALLOWED
 
         cleanup:
         jtb.tearDown()
@@ -315,58 +301,33 @@ class DimensionCacheLoaderServletSpec extends Specification {
     def "POST to addReplaceDimensionRows accepts different charsets for application/json"() {
         setup:
         JerseyTestBinder jtb = new JerseyTestBinder(DimensionCacheLoaderServlet.class)
-        Response r;
-
-        when: "POST color dimension rows with Content-Type: application/json"
-        r = jtb.getHarness().target("cache/dimensions/color/dimensionRows")
-                .request()
-                .header("Content-Type", "application/json")
-                .post(Entity.json("""
+        def postBody = Entity.json("""
                     {
                       "dimensionRows": [
                         {
                           "id": "1",
                           "description": "red"
                         }
                       ]
-                    }"""))
+                    }""")
 
-        then: "OK"
-        Status.OK.getStatusCode() == r.getStatus()
+        expect: "can POST with Content-Type: application/json"
+        jtb.getHarness().target("cache/dimensions/color/dimensionRows")
+                .request()
+                .header("Content-Type", "application/json")
+                .post(postBody).getStatusInfo() == Status.OK
 
-        when: "POST color dimension rows with Content-Type: application/json; charset=utf-8"
-        r = jtb.getHarness().target("cache/dimensions/color/dimensionRows")
+        and: "can POST with Content-Type: application/json; charset=utf-8"
+        jtb.getHarness().target("cache/dimensions/color/dimensionRows")
                 .request()
                 .header("Content-Type", "application/json; charset=utf-8")
-                .post(Entity.json("""
-                    {
-                      "dimensionRows": [
-                        {
-                          "id": "2",
-                          "description": "blue"
-                        }
-                      ]
-                    }"""))
-
-        then: "OK"
-        Status.OK.getStatusCode() == r.getStatus()
+                .post(postBody).getStatusInfo() == Status.OK
 
-        when: "POST color dimension rows with Content-Type: application/json; charset=utf-16"
-        r = jtb.getHarness().target("cache/dimensions/color/dimensionRows")
+        and: "can POST with Content-Type: application/json; charset=utf-16"
+        jtb.getHarness().target("cache/dimensions/color/dimensionRows")
                 .request()
                 .header("Content-Type", "application/json; charset=utf-16")
-                .post(Entity.json("""
-                    {
-                      "dimensionRows": [
-                        {
-                          "id": "3",
-                          "description": "green"
-                        }
-                      ]
-                    }"""))
-
-        then: "OK"
-        Status.OK.getStatusCode() == r.getStatus()
+                .post(postBody).getStatusInfo() == Status.OK
 
         cleanup:
         jtb.tearDown()

fili-core/src/test/java/com/yahoo/bard/webservice/util/ClassScanner.java

@@ -92,7 +92,8 @@ public List<Class<?>> getClasses() throws IOException, ClassNotFoundException {
             URL resource = resources.nextElement();
             dirs.add(new File(resource.getFile()));
         }
-        ArrayList<Class<?>> classes = new ArrayList<>();
+
+        List<Class<?>> classes = new ArrayList<>();
         for (File directory : dirs) {
             classes.addAll(findClasses(directory, packageName));
         }
@@ -112,14 +113,16 @@ private List<Class<?>> findClasses(File directory, String packageName) throws Cl
         if (!directory.exists()) {
             return classes;
         }
-        TreeSet<File> files = new TreeSet<>(Arrays.asList(directory.listFiles()));
 
+        Iterable<File> files = new TreeSet<>(Arrays.asList(directory.listFiles()));
         for (File file : files) {
             String name = file.getName();
             if (file.isDirectory()) {
                 assert !name.contains(".");
+                // Extend the package and recurse
                 classes.addAll(findClasses(file, packageName + "." + name));
             } else if (name.endsWith(".class")) {
+                // Grab just the class name, stripping the .class extension
                 name = packageName + '.' + name.substring(0, name.length() - 6);
                 classes.add(Class.forName(name));
             }
@@ -174,7 +177,7 @@ private <T> T constructObject(Class<T> newClass, Args mode, Stack<Class> stack)
         }
 
         // saves context for the InstantiationException
-        IllegalArgumentException cause = null;
+        IllegalArgumentException cause;
 
         // Try a no arg constructor first
         try {
@@ -277,7 +280,6 @@ private <T> T constructArg(Class<T> cls, Args mode, Stack<Class> stack) throws I
             arg = cachedValue;
         } else if (cls == Object[].class) {
             arg = (T) new Object[0];
-
         } else if (cls.isArray()) {
             Object arrayElement = constructArg(cls.getComponentType(), mode, stack);
             arg = (T) Array.newInstance(cls.getComponentType(), 1);
@@ -287,7 +289,7 @@ private <T> T constructArg(Class<T> cls, Args mode, Stack<Class> stack) throws I
         } else {
             try {
                 arg = cls.newInstance();
-            } catch (Throwable e) {
+            } catch (Throwable ignored) {
                 try {
                     arg = constructObject(cls, mode, stack);
                 } catch (Throwable e2) {
@@ -389,8 +391,8 @@ private <T> T getCachedValue(Class<T> cls) {
      *
      * @param values  The value being cached
      */
-    public void putInArgumentValueCache(Collection values) {
-        values.stream().forEach(value -> putInArgumentValueCache(value.getClass(), value));
+    public void putInArgumentValueCache(Collection<?> values) {
+        values.forEach(value -> putInArgumentValueCache(value.getClass(), value));
     }
 
     /**

fili-system-config/src/main/java/com/yahoo/bard/webservice/config/ConfigResourceLoader.java

@@ -93,8 +93,9 @@ public Configuration loadConfigFromResource(Resource resource) {
             result.load(resource.getInputStream());
             return result;
         } catch (ConfigurationException | IOException e) {
-            LOG.error(CONFIGURATION_LOAD_ERROR.format(resource.getFilename()), e);
-            throw new SystemConfigException(CONFIGURATION_LOAD_ERROR.format(resource.getFilename()), e);
+            String message = CONFIGURATION_LOAD_ERROR.format(resource.getFilename());
+            LOG.error(message, e);
+            throw new SystemConfigException(message, e);
         }
     }
 
@@ -108,7 +109,7 @@ public Configuration loadConfigFromResource(Resource resource) {
     public boolean isResourceNotAJar(Resource resource) {
         try {
             return !resource.getURI().getScheme().equals("jar");
-        } catch (IOException e) {
+        } catch (IOException ignored) {
             // If the resource doesn't parse cleanly as a URI, it's not a jar
             return true;
         }