Addressing comments

yahoo · Oct 13, 2016 · fcdad74 · fcdad74
1 parent 966c57f
commit fcdad74
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -14,6 +14,11 @@ Current
 
 ### Changed:
 
+- [Allow configurable headers for Druid data requests](https:://github.com/yahoo/fili/pull/62)
+    - The `AsyncDruidWebServiceImpl` now accepts a `Supplier<Map<String, String>>` argument which specifies
+        the headers to add to the Druid data requests. This feature is made configurable through `SystemConfig` in
+        the `AbstractBinderFactory`
+
 - [Improves error messages when querying Druid goes wrong](https:://github.com/yahoo/fili/pull/61)
     - The `ResponseException` now includes a message that prints the `ResponseException`'s internal state 
         (i.e. the druid query and response code) using the error messages 
@@ -126,6 +131,11 @@ Jobs resource. Here are the highlights of what's in this release:
 
 ### Deprecated:
 
+- [Allow configurable headers for Druid data requests](https:://github.com/yahoo/fili/pull/62)
+  * Deprecated `AsyncDruidWebServiceImpl(DruidServiceConfig, ObjectMapper)` and
+    `AsyncDruidWebServiceImpl(DruidServiceConfig, AsyncHttpClient, ObjectMapper)` because we added new construstructors
+    that take a `Supplier` argument for Druid data request headers.
+
 - [QueryTimeLookup Functionality Testing](https://github.com/yahoo/fili/pull/34)
   * Deprecated `KeyValueDimensionLoader`, in favor of `TypeAwareDimensionLoader`
 

diff --git a/fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java b/fili-core/src/main/java/com/yahoo/bard/webservice/application/AbstractBinderFactory.java
@@ -110,7 +110,6 @@
 
 import java.io.IOException;
 import java.lang.reflect.Constructor;
-import java.lang.reflect.InvocationTargetException;
 import java.time.Clock;
 import java.time.ZoneId;
 import java.util.Arrays;
@@ -942,15 +941,24 @@ protected Class<? extends PhysicalTableResolver> getPhysicalTableResolver() {
      * @return A DruidWebService
      */
     protected DruidWebService buildDruidWebService(DruidServiceConfig druidServiceConfig, ObjectMapper mapper) {
+        Supplier<Map<String, String>> supplier = buildDruidWebServiceHeaderSupplier();
+        return new AsyncDruidWebServiceImpl(druidServiceConfig, mapper, supplier);
+    }
+
+    /**
+     * Build the Supplier for Druid data request headers.
+     *
+     * @return The Druid data request header Supplier.
+     */
+    protected Supplier<Map<String, String>> buildDruidWebServiceHeaderSupplier() {
         Supplier<Map<String, String>> supplier = HashMap::new;
         String customSupplierClassString = SYSTEM_CONFIG.getStringProperty(DRUID_HEADER_SUPPLIER_CLASS, null);
         if (customSupplierClassString != null && customSupplierClassString != "") {
             try {
                 Class<?> c = Class.forName(customSupplierClassString);
                 Constructor<?> constructor = c.getConstructor();
                 supplier = (Supplier<Map<String, String>>) constructor.newInstance();
-            } catch (ClassNotFoundException | NoSuchMethodException |
-                    IllegalAccessException | InstantiationException | InvocationTargetException e) {
+            } catch (Exception e) {
                 LOG.error(
                         "Unable to load the Druid query header supplier, className: {}, exception: {}",
                         customSupplierClassString,
@@ -959,7 +967,7 @@ protected DruidWebService buildDruidWebService(DruidServiceConfig druidServiceCo
                 throw new IllegalStateException(e);
             }
         }
-        return new AsyncDruidWebServiceImpl(druidServiceConfig, mapper, supplier);
+        return supplier;
     }
 
     /**

diff --git a/...e/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java b/...e/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java
@@ -72,6 +72,8 @@ public class AsyncDruidWebServiceImpl implements DruidWebService {
      *
      * @param serviceConfig  Configuration for the Druid Service
      * @param mapper  A shared jackson object mapper resource
+     * @deprecated We now require a header supplier parameter.
+     *      Use {@link #AsyncDruidWebServiceImpl(DruidServiceConfig, ObjectMapper, Supplier)}
      */
     @Deprecated
     public AsyncDruidWebServiceImpl(
@@ -123,6 +125,8 @@ private static AsyncHttpClient initializeWebClient(int requestTimeout) {
      * @param config  the configuration for this druid service
      * @param asyncHttpClient  the HTTP client
      * @param mapper  A shared jackson object mapper resource
+     * @deprecated  We now require a header supplier parameter.
+     *      Use {@link #AsyncDruidWebServiceImpl(DruidServiceConfig, AsyncHttpClient, ObjectMapper, Supplier)}
      */
     @Deprecated
     public AsyncDruidWebServiceImpl(