address comments

yahoo · May 16, 2017 · 2f87c3d · 2f87c3d
1 parent 6b590bc
commit 2f87c3d
Show file tree

Hide file tree

Showing 3 changed files with 12 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,8 +15,8 @@ Current
     * Add new response error messages as needed by Partial Data V2
 
 - [Merge Druid Response Header into Druid Response Body Json Node in AsyncDruidWebServiceImplV2](https://github.com/yahoo/fili/pull/267)
-    * Make the use of BinderFactory as an injected behavior. By using JSON node builder strategy, we can customize the
-    content of the JSON response
+    * Add configuration to `AsyncDruidWebServiceImpl` so that we can opt-in configuration of JSON response content.
+    * `AsyncDruidWebServiceImpl` takes a strategy for building the JSON from the entire response.
 
 - [Add MetricUnionCompositeTableDefinition](https://github.com/yahoo/fili/pull/258)
 

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
@@ -66,11 +66,14 @@ public class AsyncDruidWebServiceImpl implements DruidWebService {
     public static final String DRUID_WEIGHTED_QUERY_TIMER = DRUID_TIMER + "_W_";
     public static final String DRUID_SEGMENT_METADATA_TIMER = DRUID_TIMER + "_S_0";
 
+    /**
+     * The default JSON builder puts only response body in the JSON response.
+     */
     public static final Function<Response, JsonNode> DEFAULT_JSON_NODE_BUILDER_STRATEGY =
             new Function<Response, JsonNode>() {
 
         @Override
-        public JsonNode apply(final Response response) {
+        public JsonNode apply(Response response) {
             try {
                 return new MappingJsonFactory().createParser(response.getResponseBodyAsStream()).readValueAsTree();
             } catch (IOException ioe) {
@@ -126,6 +129,8 @@ public AsyncDruidWebServiceImpl(
 
     /**
      * Friendly non-DI constructor useful for manual tests.
+     * <p>
+     * This constructor uses default JSON builder, which only uses response body to build the JSON response.
      *
      * @param serviceConfig  Configuration for the Druid Service
      * @param mapper  A shared jackson object mapper resource
@@ -167,8 +172,11 @@ public AsyncDruidWebServiceImpl(
                 jsonNodeBuilderStrategy
         );
     }
+
     /**
      * IOC constructor.
+     * <p>
+     * This constructor uses default JSON builder, which only uses response body to build the JSON response.
      *
      * @param config  the configuration for this druid service
      * @param asyncHttpClient  the HTTP client
@@ -484,16 +492,4 @@ private void markError(Status status, Response response, String druidQueryId, Ht
                 response.getResponseBody()
         );
     }
-
-    /**
-     * Extract relevant information from response and wrap them into a JSON node.
-     *
-     * @param response  The response from which the information is to be retrieved.
-     *
-     * @return the relevant information in a JSON node
-     * @throws IOException when there is a JSON parsing error
-     */
-    protected JsonNode constructJsonResponse(Response response) throws IOException {
-        return new MappingJsonFactory().createParser(response.getResponseBodyAsStream()).readValueAsTree();
-    }
 }
diff --git a/...in/java/com/yahoo/bard/webservice/druid/client/impl/HeaderNestingJsonBuilderStrategy.java b/...in/java/com/yahoo/bard/webservice/druid/client/impl/HeaderNestingJsonBuilderStrategy.java
@@ -27,7 +27,7 @@ public HeaderNestingJsonBuilderStrategy(Function<Response, JsonNode> baseStrateg
     }
 
     @Override
-    public JsonNode apply(final Response response) {
+    public JsonNode apply(Response response) {
         ObjectNode objectNode = JsonNodeFactory.instance.objectNode();
         objectNode.set("response", baseStrategy.apply(response));
         objectNode.put("X-Druid-Response-Context", response.getHeader("X-Druid-Response-Context"));