Implement DruidPartialDataResponseProcessor #275

QubitPi · 2017-05-17T16:52:59Z

3rd PR implementing #215

cdeszaq

Changelog and tests needed.

cdeszaq · 2017-05-18T17:58:46Z

...re/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/FullResponseProcessor.java

+/**
+ * Response processor for partial data V2 and cache data V3.
+ * <p>
+ * The full response processor also process header information about query response.


This description doesn't tell us very much. It's not clear what "partial data v2" or "cache data v3" are (or at least it won't be clear to us 6 months from now). This doc is also rather vague about what it does or is for, since "process header information" doesn't really indicate what it's doing or why this class is interesting.

cdeszaq · 2017-05-18T17:59:12Z

...re/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/FullResponseProcessor.java

+ * <p>
+ * The full response processor also process header information about query response.
+ */
+public interface FullResponseProcessor extends ResponseProcessor {


Why is this called "Full"? The doc should describe what "full" means.

Explanation added

cdeszaq · 2017-05-18T18:00:54Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+        validateJsonResponse(json);
+
+        int statusCode = json.get("status-code").asInt();
+        if (statusCode == 200) {


We should replace this status code number with one of the StatusCode enums. We do this elsewhere in the codebase I think

cdeszaq · 2017-05-18T18:01:37Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+
+        int statusCode = json.get("status-code").asInt();
+        if (statusCode == 200) {
+            // implementation is blocked by https://github.com/yahoo/fili/pull/262


If this is still WIP because it's not complete (which this comment seems to indicate), we should add the WIP label. Is this PR still in-progress?

@cdeszaq Since this PR is blocked by another PR, my thought is that we could review what we have now to save the reviewing effort latter. As a result, we should be able to land this PR quicker.

Since this PR does not really do anything, there's no point in landing it w/o what it's really supposed to do.

Yeah. This PR won't merge even after review. I'll wait until the blocking PR is merged and every feature is implemented and reviewed in this PR. I'm giving out the first 50% to review, so people don't have to review large chunk all together latter.

:) I'm all for getting reviews on as much as possible as soon as possible, as long as what's getting reviewed is likely to be stable against the rest of the work.

cdeszaq · 2017-05-18T18:03:35Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     * Validates JSON response object to make sure it contains all of the following information.
+     * <ul>
+     *     <li>X-Druid-Response-Context</li>
+     *     <ol>


I don't think these list items are nesting the way you intend them to, so I don't think it will render correctly. I think the closing </li> with the header name on it should go after the closing </ol> for this list.

cdeszaq · 2017-05-18T18:04:19Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     *         <li>uncoveredIntervals</li>
+     *         <li>uncoveredIntervalsOverflowed</li>
+     *     </ol>
+     *     <li>status-code</li>


Why does the status code matter?

If status code is 304, do nothing. It status code is 200, we do all logics of partial data V2

Precisely. The javadoc should say that

cdeszaq · 2017-05-18T18:05:44Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+            logAndThrowRunTimeException("Response is missing X-Druid-Response-Context");
+        }
+        if (!json.get("X-Druid-Response-Context").has("uncoveredIntervals")) {
+            logAndThrowRunTimeException("Response is missing \"uncoveredIntervals\" X-Druid-Response-Context");


Use single quotes rather than escaping the double quotes.

Also, all of these error messages really should go in the ErrorMessages enum (or whatever it's called)

And lastly, does this exist in the query if there are no uncovered intervals?

It will always exist with 98% confidence, I can double check once I get my Http Requester authentication working again.

cdeszaq · 2017-05-18T18:09:04Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     * @param message  The error message passed to the logger and the exception.
+     */
+    private static void logAndThrowRunTimeException(String message) {
+        LOG.error(message);


I think this should just be a warning, and shouldn't thrown an exception. If, for some reason, Druid doesn't give us this info, I think this processor should warn, but not assume the result is bad.

@michael-mclawhorn or @garyluoex, do you have any thoughts on this?

I think my original intent is to return a 500 error response or similar stating that we are not confident that the data druid gave us match the api user's expectation, after talking with tom, I think we agree that fail the query instead of giving bad result is the way to go. If we dont fail the query, it defeat the whole purpose of PartialDataV2 right?

That said, I don't know if there is a difference between throwing an exception directly here in comparison to preparing an actual error response by callinggetErrorCallback? (Since throwing exception means the query failed anyways and the result will most likely be a error response anyways?) @cdeszaq

In general, if a chunk of code can handle an exceptional case, it should handle it, rather than making someone else handle it. The reason for this is that, if you can handle the exception, you likely have more information about what's wrong than someone else does and can provide a better error response. (ie. "something bad happened" is far less useful than "druid indicated more data was missing than we expected" and including the additional missing intervals)

@cdeszaq @garyluoex OK. I guess we agree to keep as it is now?

No, this class needs to handle it's response. It's not useful for it to just throw if it has a problem that should fail the request.

As for the log message, it's OK to leave it error

@QubitPi Call error call back on the response instead of throw an exception, like this here

@garyluoex @cdeszaq Thanks!

cdeszaq · 2017-05-18T18:10:19Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+
+    @Override
+    public void processResponse(JsonNode json, DruidAggregationQuery<?> query, LoggingContext metadata) {
+        validateJsonResponse(json);


This doesn't handle the validator throwing an exception. If this response processor decides to fail the request, it needs to handle doing that failure, rather than letting someone else handle it. Likely it should call either the error or failure callback (depending on which makes more sense).

michael-mclawhorn · 2017-05-19T19:35:19Z

What is the thinking behind having an empty interface and leaving 'v2' in the concrete class name? Why make an empty interface? Why not just rename the response processor?

QubitPi · 2017-05-22T19:07:11Z

@cdeszaq @garyluoex @michael-mclawhorn Shall we avoid hardcoded string like

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

Line 180 in 6fb4b59

    
           if (!json.get("X-Druid-Response-Context").has("uncoveredIntervalsOverflowed")) {

? I accessed them multiple times during implementation

garyluoex · 2017-05-23T15:59:47Z

@QubitPi Lets just make them a static string on the top of the class for now, I do not see a good place to put then in at this point.

garyluoex · 2017-05-23T16:13:45Z

CHANGELOG.md

+- [Implement PartialDataV2ResponseProcessor](https://github.com/yahoo/fili/pull/275)
+    * Add `FullResponseProcessor` interface that extends `ResponseProcessor`
+    * Add response status code to JSON response
+    * Add `PartialDataV2ResponseProcessor` that checks for any missing data that's not being found


Update name to DruidPartialData

garyluoex · 2017-05-23T16:25:05Z

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

@@ -30,15 +30,20 @@ public HeaderNestingJsonBuilderStrategy(Function<Response, JsonNode> baseStrateg

    @Override
    public JsonNode apply(Response response) {
+        MappingJsonFactory mappingJsonFactory = new MappingJsonFactory();
        ObjectNode objectNode = JsonNodeFactory.instance.objectNode();
        objectNode.set("response", baseStrategy.apply(response));


We should some how unify the naming convention of 'response', 'X-Druid-Response-Context', 'status-code' by using an enum. Maybe put the num in responseprocessors folder and name it something like FullResponseContentKeys

Beautiful! I assume the static string you mentioned above is not needed anymore then

Use DruidJsonResponseContentKeys

garyluoex · 2017-05-23T16:29:37Z

...re/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/FullResponseProcessor.java

+ * <p>
+ * This is a "Full" response process in a sense that it extracts and incorporates header information.
+ */
+public interface FullResponseProcessor extends ResponseProcessor {


Try if we can override processResponse to instead of taking a JsonNode, make it take a ObjectNode in FullResponseProcessor

@garyluoex processResponse is not found to use ObjectNode. Do we still need this change?

garyluoex · 2017-05-23T16:30:10Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+ * missing interval list, we can send back an error response indicating the mismatch in data availability before the
+ * response is cached.
+ */
+public class PartialDataV2ResponseProcessor implements FullResponseProcessor {


Change name to DruidParitalData

garyluoex · 2017-05-23T16:31:47Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+ * from Partial Data V1. If "uncoveredIntervals" contains any interval that is not present in our expected
+ * missing interval list, we can send back an error response indicating the mismatch in data availability before the
+ * response is cached.
+ */


Can we slice this comment down a bit..? It feels like it is a bit too long, and if we don't simplify a little bit, it feels like we can add a phd thesis here... and it doesn't feel right to add too much detail here in the code. As long as the comment can provide a simple explanation and point to the correct place if the user wants more information, I think that is sufficient. But this is my personal preference before we come up with a rule for it, so feel free to leave it here.

Simplified.

garyluoex · 2017-05-23T16:39:00Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     * @param metadata  The LoggingContext to use
+     */
+    @Override
+    public void processResponse(JsonNode json, DruidAggregationQuery<?> query, LoggingContext metadata) {


Hopefully we can make this take a ObjectNode using the FullResponseProcessor interface as mentioned above.

garyluoex · 2017-05-23T16:40:58Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+                    query.getDataSource().getPhysicalTable().getAvailableIntervals()
+            );
+            if (!overlap.isEmpty()) {
+                logAndGetErrorCallback(ErrorMessageFormat.DATA_AVAILABILITY_MISMATCH.format(overlap), query);


We need a test for this behavior.

garyluoex · 2017-05-23T16:45:39Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     *
+     * @return uncovered intervals in a SimplifiedIntervalList.
+     */
+    private static SimplifiedIntervalList getUncoveredIntervalsFromResponse(JsonNode json) {


It should take a ObjectNode

garyluoex · 2017-05-23T16:47:35Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     */
+    private void validateJsonResponse(JsonNode json, DruidAggregationQuery<?> query) {
+        if (!json.has("X-Druid-Response-Context")) {
+            logAndGetErrorCallback("Response is missing X-Druid-Response-Context", query);


There is one more case that this statement will be executed, the case where an arrayNode is passed in instead of objectNod. The error message should note.

@garyluoex I add a check, you can see it in the next commit

garyluoex · 2017-05-23T16:48:36Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+            logAndGetErrorCallback("Response is missing X-Druid-Response-Context", query);
+        }
+        if (!json.get("X-Druid-Response-Context").has("uncoveredIntervals")) {
+            logAndGetErrorCallback("Response is missing 'uncoveredIntervals' X-Druid-Response-Context", query);


Make the message clearer by adding 'uncoveredIntervals' from 'X-Druid-Response-Context' header

michael-mclawhorn

There is a lot more testing needed here.
Also, the classes should be renamed around the non numbered naming scheme.

michael-mclawhorn · 2017-05-23T16:21:26Z

...re/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/FullResponseProcessor.java

+ * <p>
+ * This is a "Full" response process in a sense that it extracts and incorporates header information.
+ */
+public interface FullResponseProcessor extends ResponseProcessor {


Since this thing is creating an explicit contract with the json, let's document the json contract in the class documentation. e.g. give some sample json describing the nesting strategy.

michael-mclawhorn · 2017-05-23T16:24:10Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     *         Compare both SimplifiedIntervalLists above, if allAvailableIntervals has any overlap with
+     *         uncoveredIntervals, invoke error response indicating druid is missing some data that are we expects to
+     *         exists.
+     *     </li>


spelling: 'that we are expecting'

michael-mclawhorn · 2017-05-23T16:25:33Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+     * @param query  The query with the schema for processing this response
+     */
+    private void validateJsonResponse(JsonNode json, DruidAggregationQuery<?> query) {
+        if (!json.has("X-Druid-Response-Context")) {


Extract as constant

michael-mclawhorn · 2017-05-23T16:27:22Z

...in/java/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessor.java

+        if (json.get("X-Druid-Response-Context").get("uncoveredIntervalsOverflowed").asBoolean()) {
+            int limit = query.getContext().getUncoveredIntervalsLimit();
+            logAndGetErrorCallback(ErrorMessageFormat.TOO_MUCH_INTERVAL_MISSING.format(limit, limit), query);
+        }


grammar: either 'TOO_MANY_INTERVALS' or 'TOO_MUCH_TIME'

Also, 'TOO_MUCH_INTERVAL_MISSING' really shouldn't need the same number twice.
It can probably be reworded as:
"Query is returning more than the configured limit of ‘%s’ missing intervals. There may be a problem with your data."
I don't think end users should ever be contemplating reconfiguring this parameter.

michael-mclawhorn · 2017-05-23T18:28:04Z

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

@@ -50,7 +51,8 @@ class AsyncDruidWebServiceImplWithHeaderNestingSpec extends Specification {
                                "2016-12-25T00:00:00.000Z/2017-01-03T00:00:00.000Z"
                            ],
                            "uncoveredIntervalsOverflowed": true
-                        }
+                        },
+                        "status-code": 200


it looks to me like this is a test of the HeaderNestingJsonBuilderStrategy, not of the webservice itself.

Yeah. I agree. I think I should rename it to make it correspond to HeaderNestingJsonBuilderStrategy.java

michael-mclawhorn · 2017-05-23T18:28:57Z

...y/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessorSpec.groovy

+        1 * next.getResponseContext() >> responseContext
+        1 * next.getFailureCallback(druidAggregationQuery) >> failureCallback
+        1 * next.getErrorCallback(druidAggregationQuery) >> httpErrorCallback
+    }


What is this even testing? I don't see the point of this.

michael-mclawhorn · 2017-05-23T18:30:15Z

...y/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessorSpec.groovy

+            "status-code": 200
+        }
+        ''' | ["2016-11-22T00:00:00.000Z/2016-12-18T00:00:00.000Z", "2016-12-25T00:00:00.000Z/2017-01-03T00:00:00.000Z"]
+    }


This is a good test. However if we're only doing one value, there's not much point in doing a data table.

michael-mclawhorn · 2017-05-23T18:33:44Z

...y/com/yahoo/bard/webservice/web/responseprocessors/PartialDataV2ResponseProcessorSpec.groovy

+        }
+        ''' | ["2016-11-22T00:00:00.000Z/2016-12-18T00:00:00.000Z", "2016-12-25T00:00:00.000Z/2017-01-03T00:00:00.000Z"]
+    }
+}


I assume there are more tests coming here.
Specifically:
processResponse -> overflow message
processResponse -> success message
processResponse -> data availability mismatch
validateJsonResponse error paths
checkOverflow success and failure
getUncoveredIntervalFromResponse (optional, but might make for good low level testing)

garyluoex

Minor stuff

garyluoex · 2017-05-25T17:56:21Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

@@ -176,19 +152,29 @@ public void processResponse(JsonNode json, DruidAggregationQuery<?> query, Loggi
     * @param query  The query with the schema for processing this response
     */
    private void validateJsonResponse(JsonNode json, DruidAggregationQuery<?> query) {
-        if (!json.has("X-Druid-Response-Context")) {
+        if (json instanceof ArrayNode) {


Use getNodeType, https://fasterxml.github.io/jackson-databind/javadoc/2.7/com/fasterxml/jackson/databind/JsonNode.html#getNodeType().

garyluoex · 2017-05-25T17:59:45Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

@@ -176,19 +152,29 @@ public void processResponse(JsonNode json, DruidAggregationQuery<?> query, Loggi
     * @param query  The query with the schema for processing this response
     */
    private void validateJsonResponse(JsonNode json, DruidAggregationQuery<?> query) {
-        if (!json.has("X-Druid-Response-Context")) {
+        if (json instanceof ArrayNode) {
+            logAndGetErrorCallback("Response is missing X-Druid-Response-Context and status code", query);


I think we should make the message more explicit such that Response refers to json being passed to ResponseHandler so that people don't mistake it as being Response from druid. Apply to all.

garyluoex · 2017-05-25T18:04:14Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

            logAndGetErrorCallback("Response is missing X-Druid-Response-Context", query);
        }
-        if (!json.get("X-Druid-Response-Context").has("uncoveredIntervals")) {
-            logAndGetErrorCallback("Response is missing 'uncoveredIntervals' X-Druid-Response-Context", query);
+        if (!json.get(FullResponseContentKeys.DRUID_RESPONSE_CONTEXT.getName())


It is repeated four or more times, we should take it out as its own variable

garyluoex · 2017-05-25T18:05:54Z

.../src/main/java/com/yahoo/bard/webservice/web/responseprocessors/FullResponseContentKeys.java

+/**
+ * Enumerates the list of keys expected to be found in the FullResponseProcessor.
+ */
+public enum FullResponseContentKeys {


Lets change it to DruidJsonResponseContentKeys?

@garyluoex Shall we change FullResponseProcessor to DruidJsonResponseProcessor?

garyluoex · 2017-05-25T19:12:46Z

...re/src/main/java/com/yahoo/bard/webservice/web/responseprocessors/FullResponseProcessor.java

@@ -39,4 +47,21 @@
 *
 */
 public interface FullResponseProcessor extends ResponseProcessor {
+    @Override
+    default void processResponse(JsonNode json, DruidAggregationQuery<?> query, LoggingContext metadata) {


Why do we have a default?

The only way to override is to implement processResponse and default is needed to implement this method

An alternative is to tranform JSON node to Object node in DruidPartialDataResponseProcessor

Are you sure? I think just overriding the method as an interface without default should be fine. Let me know the compilation error you get if possible.

never mind, revert it

garyluoex · 2017-05-26T02:11:24Z

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

@@ -30,15 +30,20 @@ public HeaderNestingJsonBuilderStrategy(Function<Response, JsonNode> baseStrateg

    @Override
    public JsonNode apply(Response response) {
+        MappingJsonFactory mappingJsonFactory = new MappingJsonFactory();
        ObjectNode objectNode = JsonNodeFactory.instance.objectNode();
        objectNode.set("response", baseStrategy.apply(response));
        try {
            objectNode.set(
                    "X-Druid-Response-Context",


DruidJsonResponseContentKeys

garyluoex · 2017-05-26T02:11:28Z

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

                            .createParser(response.getHeader("X-Druid-Response-Context"))
                            .readValueAsTree()
            );
+            objectNode.set(
+                    "status-code",


DruidJsonResponseContentKeys

garyluoex

Minor comment

garyluoex · 2017-05-26T15:44:22Z

...main/java/com/yahoo/bard/webservice/web/responseprocessors/DruidJsonResponseContentKeys.java

+    UNCOVERED_INTERVALS("uncoveredIntervals"),
+    UNCOVERED_INTERVALS_OVERFLOWED("uncoveredIntervalsOverflowed"),
+    STATUS_CODE("status-code"),
+    RESPONSE("response");


semi colon to next line

garyluoex · 2017-05-26T15:44:42Z

...om/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessorSpec.groovy

+        )
+        dataSource.getPhysicalTable() >> constrainedTable
+        druidAggregationQuery.getDataSource() >> dataSource
+


extra new line

garyluoex · 2017-05-26T15:46:51Z

...om/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessorSpec.groovy

+        JsonNode json = MAPPER.readTree(
+                jsonInString
+                        .replace(" ", "")
+                        .replace("\n", "")


why are these two replace needed?

Good catch! They are not needed after a refactor but I forget to delete them

garyluoex · 2017-05-26T15:53:19Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

+            }
+        }
+
+        next.processResponse(json, query, metadata);


If it is 304 and the next response processor is not fullResponseProcessor, we should throw error saying, received 304, but no etag cache response processor is available to process 304 response

garyluoex · 2017-05-26T15:59:09Z

...om/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessorSpec.groovy

+                    "2016-11-22T00:00:00.000Z/2016-12-18T00:00:00.000Z",
+                    "2016-12-25T00:00:00.000Z/2017-01-03T00:00:00.000Z"
+                ],
+                "uncoveredIntervalsOverflowed": true


Why are all over flow set to true????

And.... this test only have a given: ????

OK. Setting it to false makes more sense

garyluoex · 2017-05-26T16:01:05Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

+                json.get(DruidJsonResponseContentKeys.DRUID_RESPONSE_CONTEXT.getName())
+                        .get(DruidJsonResponseContentKeys.UNCOVERED_INTERVALS.getName())
+                ) {
+            intervals.add(new Interval(jsonNode.asText()));


Can we add a test showing what this jsonNode is? Or let me know if you know the answer, is this jsonNode a arraynode or objectnode and what is the content?

According to Javadoc of this method

X-Druid-Response-Context: { "uncoveredIntervals": [ "2016-11-22T00:00:00.000Z/2016-12-18T00:00:00.000Z","2016-12-25T00:00:00.000Z/2017- 01-03T00:00:00.000Z","2017-01-31T00:00:00.000Z/2017-02-01T00:00:00.000Z","2017-02- 08T00:00:00.000Z/2017-02-09T00:00:00.000Z","2017-02-10T00:00:00.000Z/2017-02- 13T00:00:00.000Z","2017-02-16T00:00:00.000Z/2017-02-20T00:00:00.000Z","2017-02- 22T00:00:00.000Z/2017-02-25T00:00:00.000Z","2017-02-26T00:00:00.000Z/2017-03- 01T00:00:00.000Z","2017-03-04T00:00:00.000Z/2017-03-05T00:00:00.000Z","2017-03- 08T00:00:00.000Z/2017-03-09T00:00:00.000Z" ], "uncoveredIntervalsOverflowed": true }

Each is a ObjectNode containing a String like "2016-11-22T00:00:00.000Z/2016-12-18T00:00:00.000Z"

If it is a ObjectNode, it is some kind of key value mapping right? So what is the key for each individual string like "2016-11-22T00:00:00.000Z/2016-12-18T00:00:00.000Z"?

What I mean by ObjectNode is that it is a type of JsonNode that can be serialized to text in this way

Can you show me some documentation or a test that confirms this behavior?

I modified tests to make sure this method works as expected

Isn't this a text node inside a list?

Re: @michael-mclawhorn Yes, which is why I call .asText(). Any specific problem?

garyluoex · 2017-05-26T16:17:00Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

@@ -131,9 +132,14 @@ public void processResponse(JsonNode json, DruidAggregationQuery<?> query, Loggi
            } else {
                next.processResponse(json.get(DruidJsonResponseContentKeys.RESPONSE.getName()), query, metadata);
            }
+        } else if (statusCode == Status.NOT_MODIFIED.getStatusCode() && next instanceof FullResponseProcessor) {


Are you sure this if statement is correct?

Maybe adding a pair of parenthesis around latter?

if 304, and the next response Processor is FullResponseProcessor (which can be etagCacheResponseProcessor), we throw an error?

garyluoex · 2017-05-26T16:27:14Z

...om/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessorSpec.groovy

+        druidPartialDataResponseProcessor.next == next;
+    }
+
+    def "getOverlap returns intersection between Druid intervals and Fili intervals in case of #caseDescription"() {


This test only have a given: and where: which does not test anything right?

Hmm...somehow a refactor removes it. Anyway it's back

garyluoex · 2017-05-26T16:27:30Z

...oovy/com/yahoo/bard/webservice/druid/client/impl/HeaderNestingJsonBuilderStrategySpec.groovy

@@ -50,7 +51,8 @@ class AsyncDruidWebServiceImplWithHeaderNestingSpec extends Specification {
                                "2016-12-25T00:00:00.000Z/2017-01-03T00:00:00.000Z"
                            ],
                            "uncoveredIntervalsOverflowed": true
-                        }
+                        },
+                        "status-code": 200
                    }
                '''.replace(" ", "").replace("\n", "")


Not sure if this is needed.

garyluoex · 2017-05-26T16:27:39Z

...oovy/com/yahoo/bard/webservice/druid/client/impl/HeaderNestingJsonBuilderStrategySpec.groovy

@@ -28,6 +28,7 @@ class AsyncDruidWebServiceImplWithHeaderNestingSpec extends Specification {
                        "uncoveredIntervalsOverflowed": true
                    }
                '''.replace(" ", "").replace("\n", "")


Not sure if this is needed.

michael-mclawhorn

Readability of tests should be improved, otherwise this is pretty good. I'm going to 👍 this, but I'd like you to make the changes I've described.

michael-mclawhorn · 2017-05-26T23:22:25Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

+                json.get(DruidJsonResponseContentKeys.DRUID_RESPONSE_CONTEXT.getName())
+                        .get(DruidJsonResponseContentKeys.UNCOVERED_INTERVALS.getName())
+                ) {
+            intervals.add(new Interval(jsonNode.asText()));


Isn't this a text node inside a list?

Re: @michael-mclawhorn Yes, which is why I call .asText(). Any specific problem?

michael-mclawhorn · 2017-05-26T23:29:13Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

+    private void validateJsonResponse(JsonNode json, DruidAggregationQuery<?> query) {
+        if (json.getNodeType() == JsonNodeType.ARRAY) {
+            logAndGetErrorCallback("JSON response is missing X-Druid-Response-Context and status code", query);
+        }


All of these error strings should be moved into constants and referenced by the constants in the tests so that they'll stay in sync.

michael-mclawhorn · 2017-05-26T23:29:22Z

...java/com/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessor.java

+        }
+        if (!json.has(DruidJsonResponseContentKeys.STATUS_CODE.getName())) {
+            logAndGetErrorCallback("JSON response is missing response status code", query);
+        }


All of these error strings should be moved into constants and referenced by the constants in the tests so that they'll stay in sync.

michael-mclawhorn · 2017-05-26T23:30:33Z

...om/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessorSpec.groovy

+                "Query is returning more than the configured limit of '10' missing intervals. " +
+                        "There may be a problem with your data.")
+    }
+


Use the constants for these errors rather than copying them into the tests.

michael-mclawhorn · 2017-05-26T23:32:59Z

...om/yahoo/bard/webservice/web/responseprocessors/DruidPartialDataResponseProcessorSpec.groovy

+                ],
+                "uncoveredIntervalsOverflowed": false
+            },
+            "status-code": 200


It looks like your Json instring is the same across all the test cases. If so, lift it up out of the where clause and put it into the test.

QubitPi · 2017-05-30T18:20:54Z

@cdeszaq Since merging is blocked, could you take a quick look at it and approve it? Thanks!

Unblock merging.

garyluoex · 2017-05-30T18:52:09Z

@QubitPi @cdeszaq I dismissed the comment so that merging is not blocked since it's got 2 reviews already.

QubitPi · 2017-05-30T18:53:39Z

@garyluoex Thanks Gary!

QubitPi added the WIP label May 17, 2017

QubitPi force-pushed the implement-PartialDataV2ResponseProcessor branch 3 times, most recently from 7ea000c to 6547e74 Compare May 17, 2017 18:01

QubitPi mentioned this pull request May 18, 2017

Implement DruidPartialDataRequestHandler #287

Merged

QubitPi added NEED 2 REVIEWS REVIEWABLE and removed WIP labels May 18, 2017

cdeszaq previously requested changes May 18, 2017

View reviewed changes

garyluoex self-assigned this May 20, 2017

QubitPi force-pushed the implement-PartialDataV2ResponseProcessor branch 3 times, most recently from 313c265 to 6fb4b59 Compare May 22, 2017 19:03

QubitPi force-pushed the implement-PartialDataV2ResponseProcessor branch from 98ab398 to 991983b Compare May 22, 2017 19:34

garyluoex requested changes May 23, 2017

View reviewed changes

QubitPi changed the title ~~Implement PartialDataV2ResponseProcessor~~ Implement DruidPartialDataResponseProcessor May 23, 2017

michael-mclawhorn requested changes May 23, 2017

View reviewed changes

garyluoex requested changes May 25, 2017

View reviewed changes

garyluoex reviewed May 25, 2017

View reviewed changes

garyluoex reviewed May 26, 2017

View reviewed changes

QubitPi force-pushed the implement-PartialDataV2ResponseProcessor branch from 4a130e9 to 3b07f2a Compare May 26, 2017 15:45

garyluoex approved these changes May 26, 2017

View reviewed changes

garyluoex reviewed May 26, 2017

View reviewed changes

garyluoex requested changes May 26, 2017

View reviewed changes

garyluoex reviewed May 26, 2017

View reviewed changes

garyluoex requested changes May 26, 2017

View reviewed changes

garyluoex approved these changes May 26, 2017

View reviewed changes

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

QubitPi force-pushed the implement-PartialDataV2ResponseProcessor branch from 6a9c85d to 6919466 Compare May 26, 2017 18:23

Implement PartialDataV2ResponseProcessor

5131965

QubitPi force-pushed the implement-PartialDataV2ResponseProcessor branch from 6919466 to 5131965 Compare May 26, 2017 20:03

michael-mclawhorn approved these changes May 26, 2017

View reviewed changes

Address @michael-mclawhorn's comments

3a5e0e0

QubitPi force-pushed the implement-PartialDataV2ResponseProcessor branch from 9b34856 to 3a5e0e0 Compare May 27, 2017 06:50

QubitPi added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels May 30, 2017

QubitPi mentioned this pull request May 30, 2017

Prepare for etag Cache #289

Merged

QubitPi merged commit f9db95a into master May 30, 2017

garyluoex deleted the implement-PartialDataV2ResponseProcessor branch May 30, 2017 21:14

Implement DruidPartialDataResponseProcessor #275

Implement DruidPartialDataResponseProcessor #275

Conversation

QubitPi commented May 17, 2017 • edited Loading

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

QubitPi May 18, 2017 • edited Loading

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

garyluoex May 18, 2017 • edited Loading

Choose a reason for hiding this comment

garyluoex May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi May 18, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garyluoex May 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-mclawhorn commented May 19, 2017

QubitPi commented May 22, 2017

garyluoex commented May 23, 2017

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 23, 2017 • edited Loading

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

michael-mclawhorn 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

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 commented May 17, 2017 •

edited

Loading

QubitPi May 18, 2017 •

edited

Loading

garyluoex May 18, 2017 •

edited

Loading

garyluoex May 18, 2017 •

edited

Loading

QubitPi May 18, 2017 •

edited

Loading

garyluoex May 19, 2017 •

edited

Loading

QubitPi May 23, 2017 •

edited

Loading