Merge Header Info into JsonNode #267

QubitPi · 2017-05-04T17:59:21Z

2nd PR implementing #215

cdeszaq · 2017-05-05T16:42:58Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

-    private final Meter httpErrorMeter;
-    private final Meter exceptionMeter;
+    protected final Meter httpErrorMeter;
+    protected final Meter exceptionMeter;


Rather than exposing fields directly, we should use methods instead. In this case, methods to count errors, etc. are probably the right call, and accessors will likely work for the other variables made protected in this pr

garyluoex

Add change log

garyluoex · 2017-05-05T20:41:33Z

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

@@ -169,6 +170,11 @@
    public static final String DEPRECATED_PERMISSIVE_AVAILABILITY_FLAG = SYSTEM_CONFIG.getPackageVariableName(
            "permissive_column_availability_enabled");

+    public static final int DRUID_UNCOVERED_INTERVAL_LIMIT = SYSTEM_CONFIG.getIntProperty(
+            SYSTEM_CONFIG.getPackageVariableName("druid_uncovered_interval_limit"),
+            -1


Default to 0 as discussed earlier.

garyluoex · 2017-05-05T21:21:52Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -356,4 +357,12 @@ public Integer getTimeout() {
    public DruidServiceConfig getServiceConfig() {
        return serviceConfig;
    }
+
+    public Meter getHttpErrorMeter() {


should be protected.

garyluoex · 2017-05-05T21:22:00Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+        return httpErrorMeter;
+    }
+
+    public Meter getExceptionMeter() {


should be protected.

garyluoex · 2017-05-05T21:30:02Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

@@ -66,8 +67,8 @@
    public static final String DRUID_WEIGHTED_QUERY_TIMER = DRUID_TIMER + "_W_";
    public static final String DRUID_SEGMENT_METADATA_TIMER = DRUID_TIMER + "_S_0";

-    private final Supplier<Map<String, String>> headersToAppend;
-    private final DruidServiceConfig serviceConfig;
+    protected final Supplier<Map<String, String>> headersToAppend;


why this needs to be protected?

changed back to private

garyluoex · 2017-05-05T21:30:11Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

-    private final Supplier<Map<String, String>> headersToAppend;
-    private final DruidServiceConfig serviceConfig;
+    protected final Supplier<Map<String, String>> headersToAppend;
+    protected final DruidServiceConfig serviceConfig;


Why this needs to be protected?

changed back to private

garyluoex

We should probably do a test to make sure that the jsonNode we built are as what we expect it to be.

garyluoex · 2017-05-09T17:58:39Z

...re/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2.java

+                                            )
+                                    ) {
+                                        rootNode = jsonParser.readValueAsTree();
+                                        objectNode = (ObjectNode) rootNode;


Simply casting is not a good choice i think, we should create a new ObjectNode and put things into the ObjectNode.

I would try out and consider moving the response parsing into jsonNode process into its own method in AsyncDruidWebserviceImpl.java and simply override it here instead of overriding the whole sendRequest method. If we do that, we also need to move the response status check on the top of this sendRequest method into its own method and override it here also. If you think this makes sense too, try it.

Also fix the unused import things

@garyluoex It's a long method. So, yeah, let's break it down

garyluoex · 2017-05-10T20:50:30Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java

-                    "following intervals %s where druid does not have data"
-    ),
-
-    TOO_MUCH_INTERVAL_MISSING(


Why did we remove these?

They are back

garyluoex · 2017-05-10T20:50:37Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java

@@ -215,17 +215,6 @@

    RESULT_MAPPING_FAILURE(
            "Error occurred while processing response data: %s"
-    ),
-
-    DATA_AVAILABILITY_MISMATCH(


Why did we remove these?

garyluoex · 2017-05-10T20:56:09Z

...re/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2.java

+
+    @Override
+    protected JsonNode constructJsonResponse(Response response) throws IOException {
+        ObjectNode objectNode = (ObjectNode) OBJECT_MAPPER.readTree(response.getResponseBody());


The reason I suggested to create a new ObjectNode is because I suspect that the JsonNode created by

(ObjectNode) OBJECT_MAPPER.readTree(response.getResponseBody());

is actually an ArrayNode, force casting it to ObjectNode will cause runtime error. Which I want the test to test, but it seems like the test assumes that the response.getResponseBody() returns a object, but I think it is actually an array.

To create a new node,

ObjectNode node = JsonNodeFactory.instance.objectNode();

cdeszaq · 2017-05-10T18:58:33Z

fili-core/src/main/java/com/yahoo/bard/webservice/web/ErrorMessageFormat.java

-    TOO_MUCH_INTERVAL_MISSING(
-            "More than %s interval missing information received from druid, inspect if query " +
-                    "expects more than %s missing intervals or increase " +
-                    "uncoveredIntervalsLimit configuration value"


Where are these going?

They are back

cdeszaq · 2017-05-10T18:59:30Z

...est/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2Spec.groovy

+        response.getResponseBody() >>
+                """
+                    {"k1":"v1"}
+                """


Why is this using a multi-line string? Can it just be 1 line?

Also the first level should be array instead of object.

@garyluoex Could you give an example?

Yes, why should it be an array and not an object? Are you saying that Druid's response is a top-level array?

cdeszaq · 2017-05-10T19:00:29Z

...est/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2Spec.groovy

+        )
+
+        expect:
+        print asyncDruidWebServiceImplV2.constructJsonResponse(response)


Why is this here?

I forgot to delete it

cdeszaq · 2017-05-10T19:01:52Z

...est/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2Spec.groovy

+import java.util.function.Supplier;
+
+class AsyncDruidWebServiceImplV2Spec extends Specification {
+    def "Make sure X-Druid-Response-Context and status-code are mreged into existing JsonNode"() {


s/mreged/merged

cdeszaq · 2017-05-10T19:02:54Z

...re/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2.java

+ */
+public class AsyncDruidWebServiceImplV2 extends AsyncDruidWebServiceImpl {
+
+    private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();


We should not make a new mapper. We should get the same mapper that the rest of the system is using.

We also should not be using a static one.

cdeszaq · 2017-05-10T20:57:39Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+     * @throws IOException when there is a JSON parsing error
+     */
+    protected JsonNode constructJsonResponse(Response response) throws IOException {
+        MappingJsonFactory jsonFactory = new MappingJsonFactory();


Can probably be inlined?

cdeszaq · 2017-05-10T21:05:07Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+     * @param error  callback for handling http errors.
+     */
+    private void markError(Status status, Response response, String druidQueryId, HttpErrorCallback error) {
+        httpErrorMeter.mark();


Should use the getter, not the direct field access

cdeszaq · 2017-05-10T21:08:07Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+    }
+
+    /**
+     * Log request using RequestLog.


This is doing much more than just logging the request, right? We should probably call out all of those different responsibilities of this method?

It's also worth noting that this isn't logging the request, but it's logging the completed response. If it was logging the request, it would not need to be triggering with pieces of the response. And if it was for any response, it should handle the failure cases as well.

cdeszaq · 2017-05-10T21:08:57Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+
+        LOG.debug(
+                "druid {} response code: {} {} and druid query id: {}",
+                serviceConfig.getNameAndUrl(),


Should use the getter, not direct field access

cdeszaq · 2017-05-10T21:17:25Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

-                                    rootNode = jp.readValueAsTree();
-                                }
-                                success.invoke(rootNode);
+                                JsonNode jsonNode = constructJsonResponse(response);


cdeszaq · 2017-05-11T18:20:39Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+    }
+
+    /**
+     * Logs request using RequestLog and logs Druid response.


This also restores the serialized request log info back into the RequestLog object, stops / starts some timers, counts outstanding queries, etc. We should definitely call those things out, since those are very important side-effects of this method.

cdeszaq · 2017-05-11T18:24:01Z

...re/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2.java

+/**
+ * Represents the druid web service endpoint for partial data V2.
+ * <p>
+ * When Druid returns data with missing intervals with a non-OK status code, instead of


Will Druid have a non-OK response code in that case? (ie. where there are missing intervals) Or does it still return as a 200 OK?

cdeszaq · 2017-05-11T18:29:25Z

...re/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2.java

+
+        JsonNode responseNode = new MappingJsonFactory().createParser(response.getResponseBodyAsStream())
+                .readValueAsTree();
+        objectNode.put("response", responseNode);


This version of put is deprecated.

cdeszaq · 2017-05-11T18:32:17Z

...est/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2Spec.groovy

-                """
-                    {"k1":"v1"}
-                """
+        response.getResponseBodyAsStream() >> new ByteArrayInputStream("{\"k1\":\"v1\"}".getBytes(StandardCharsets.UTF_8))


Rather than escaping the quotes, we should use Strings (instead of GStrings) if we don't need the templating behavior.

garyluoex · 2017-05-12T16:34:03Z

...est/groovy/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplV2Spec.groovy

+    def "Make sure X-Druid-Response-Context and status-code are merged into existing JsonNode"() {
+        given:
+        Response response = Mock(Response)
+        response.getResponseBodyAsStream() >> new ByteArrayInputStream('{"k1":"v1"}'.getBytes(StandardCharsets.UTF_8))


The body should be an array at the top level instead of object.

garyluoex · 2017-05-16T17:39:42Z

.../main/java/com/yahoo/bard/webservice/druid/client/impl/HeaderNestingJsonBuilderStrategy.java

+/**
+ * A strategy that takes a JSON Node built from a Response, nests it and makes it a peer with the Response headers.
+ */
+public class HeaderNestingJsonBuilderStrategy implements Function<Response, JsonNode> {


Can we remove the word Strategy? It makes the name longer without adding much information I think.

@garyluoex No problem. @michael-mclawhorn Do I have the permission to remove it?

It's naming the Function that this extends from, indicating how the function is being used (ie. the Strategy pattern, to determine some behavior in a broader set of processing). This name is really 2 parts: HeaderNesting and JsonBuilderStrategy. The 1st part is describing this implementation, and the 2nd part is really what purpose this Function is serving.

So, tl;dr: I'd keep it. But that's just my opinion.

Not critical, just leave it then

garyluoex · 2017-05-16T17:40:35Z

...yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplWithHeaderNestingSpec.groovy

    def "Make sure X-Druid-Response-Context and status-code are merged into existing JsonNode"() {
        given:
        Response response = Mock(Response)
-        response.getResponseBodyAsStream() >> new ByteArrayInputStream('[{"k1":"v1"}]'.getBytes(StandardCharsets.UTF_8))
+        response.getResponseBodyAsStream() >> new ByteArrayInputStream('{"k1":"v1"}'.getBytes(StandardCharsets.UTF_8))


Rebase error, need to put back the list bracket.

garyluoex · 2017-05-16T17:41:50Z

...yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImplWithHeaderNestingSpec.groovy

                '''
                    {
-                        "response": "[{"k1":"v1"}]",
+                        "response": {"k1":"v1"},


Rebase error, need to put back the list bracket.

garyluoex · 2017-05-16T17:43:14Z

CHANGELOG.md

@@ -14,6 +14,12 @@ Current
    * Add a configurable property named "druid_uncovered_interval_limit"
    * 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)
+    * Added `AsyncDruidWebServiceImplV2` which overrides `sendRequest` method in `AsyncDruidWebServiceImpl` by adding


Need to update.

garyluoex · 2017-05-16T17:43:21Z

CHANGELOG.md

+- [Merge Druid Response Header into Druid Response Body Json Node in AsyncDruidWebServiceImplV2](https://github.com/yahoo/fili/pull/267)
+    * Added `AsyncDruidWebServiceImplV2` which overrides `sendRequest` method in `AsyncDruidWebServiceImpl` by adding
+    "X-Druid-Response-Context" to the response JSON
+    * AbstractBinderFactory::buildDruidWebService returns `AsyncDruidWebServiceImplV2` or `AsyncDruidWebServiceImplV2`


Need to update.

cdeszaq

Nothing big. Just some minor formatting, javadoc, and changelog cleanup.

cdeszaq · 2017-05-16T18:38:56Z

CHANGELOG.md

@@ -14,6 +14,10 @@ Current
    * Add a configurable property named "druid_uncovered_interval_limit"
    * 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)


It may be worth calling out here that this is not a change to the default behavior, but is instead opt-in via configuration (and including how to configure that opt-in)

Should also call out the change to the AsyncDruidWebServiceClientImpl, in that it now takes a strategy for building the JSON from the entire response. (since that's what the ability we're adding hinges on)

cdeszaq · 2017-05-16T18:40:49Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

    public static final String DRUID_TIMER = "DruidProcessing";
    public static final String DRUID_QUERY_TIMER = DRUID_TIMER + "_Q_";
    public static final String DRUID_QUERY_ALL_TIMER = DRUID_QUERY_TIMER + "All";
    public static final String DRUID_QUERY_MAX_TIMER = DRUID_QUERY_TIMER + "Max";
    public static final String DRUID_WEIGHTED_QUERY_TIMER = DRUID_TIMER + "_W_";
    public static final String DRUID_SEGMENT_METADATA_TIMER = DRUID_TIMER + "_S_0";

+    public static final Function<Response, JsonNode> DEFAULT_JSON_NODE_BUILDER_STRATEGY =


Needs JavaDoc (since this essentially a class) explaining what it does. "Default" isn't super descriptive.

cdeszaq · 2017-05-16T18:40:56Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+            new Function<Response, JsonNode>() {
+
+        @Override
+        public JsonNode apply(final Response response) {


final not needed

@cdeszaq I removed final, but why is it not needed? I'm just curious.

cdeszaq · 2017-05-16T18:42:25Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+                initializeWebClient(serviceConfig.getTimeout()),
+                mapper,
+                headersToAppend,
+                DEFAULT_JSON_NODE_BUILDER_STRATEGY


JavaDoc on this should call out that this constructor uses a particular strategy as the default.

cdeszaq · 2017-05-16T18:42:39Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

}
-

Keep 1 blank line between methods

cdeszaq · 2017-05-16T18:43:13Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

    }
-
    /**
     * IOC constructor.


JavaDoc should call out what strategy is used as the default, since this constructor doesn't take a strategy.

cdeszaq · 2017-05-16T18:47:12Z

...core/src/main/java/com/yahoo/bard/webservice/druid/client/impl/AsyncDruidWebServiceImpl.java

+     * @return the relevant information in a JSON node
+     * @throws IOException when there is a JSON parsing error
+     */
+    protected JsonNode constructJsonResponse(Response response) throws IOException {


This method is never used, and seems to have been replaced by the strategy pattern.

cdeszaq · 2017-05-16T18:48:27Z

.../main/java/com/yahoo/bard/webservice/druid/client/impl/HeaderNestingJsonBuilderStrategy.java

+    }
+
+    @Override
+    public JsonNode apply(final Response response) {


final not needed

garyluoex

Last problem, otherwise good for now.

garyluoex · 2017-05-16T18:59:47Z

.../main/java/com/yahoo/bard/webservice/druid/client/impl/HeaderNestingJsonBuilderStrategy.java

+    public JsonNode apply(final 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"));


Should use set instead of put.

put is deprecated, we should parse it as a jsonNode

Modify test accordingly also.

QubitPi added NEED 2 REVIEWS REVIEWABLE labels May 4, 2017

garyluoex self-assigned this May 4, 2017

cdeszaq requested changes May 5, 2017

View reviewed changes

QubitPi mentioned this pull request May 5, 2017

Prepare For Partial Data V2 #264

Merged

garyluoex requested changes May 5, 2017

View reviewed changes

garyluoex requested changes May 9, 2017

View reviewed changes

QubitPi force-pushed the Merge-Header-Info-into-JsonNode branch from 8df824f to 57678c3 Compare May 9, 2017 22:25

garyluoex requested changes May 10, 2017

View reviewed changes

QubitPi force-pushed the Merge-Header-Info-into-JsonNode branch from 57678c3 to e53307d Compare May 10, 2017 21:20

cdeszaq requested changes May 10, 2017

View reviewed changes

cdeszaq requested changes May 11, 2017

View reviewed changes

garyluoex requested changes May 12, 2017

View reviewed changes

garyluoex approved these changes May 12, 2017

View reviewed changes

QubitPi added NEED 1 REVIEW and removed NEED 2 REVIEWS labels May 12, 2017

garyluoex requested changes May 16, 2017

View reviewed changes

cdeszaq approved these changes May 16, 2017

View reviewed changes

garyluoex approved these changes May 16, 2017

View reviewed changes

QubitPi force-pushed the Merge-Header-Info-into-JsonNode branch from 037a96a to f6125d8 Compare May 16, 2017 20:07

QubitPi added 2 commits May 16, 2017 13:07

Merge Header Info into JsonNode

6b590bc

address comments

2f87c3d

QubitPi force-pushed the Merge-Header-Info-into-JsonNode branch from f6125d8 to 2f87c3d Compare May 16, 2017 20:07

address @garyluoex's comments

4a53ee9

QubitPi merged commit c9a7df2 into master May 16, 2017

QubitPi deleted the Merge-Header-Info-into-JsonNode branch May 16, 2017 20:32

QubitPi added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels Jul 19, 2017

Merge Header Info into JsonNode #267

Merge Header Info into JsonNode #267

Conversation

QubitPi commented May 4, 2017 • edited

Choose a reason for hiding this comment

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi May 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex May 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi May 16, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdeszaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi commented May 4, 2017 •

edited

QubitPi May 9, 2017 •

edited

garyluoex May 10, 2017 •

edited

QubitPi May 16, 2017 •

edited