Implement EtagCacheResponseProcessor #311

QubitPi · 2017-05-31T17:48:07Z

The 2nd PR implementing #255

michael-mclawhorn

Needs tests.

michael-mclawhorn · 2017-06-02T15:54:23Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+            try {
+                ((ObjectNode) json).set(
+                        DruidJsonResponseContentKeys.RESPONSE.getName(),
+                        mapper.readTree(dataCache.getDataValue(DruidJsonResponseContentKeys.RESPONSE.getName()))


It might be slightly more efficient to keep this around rather than reserialize on cache hit. But I don't think it matters much.

Did you mean assigning it to a variable?

cdeszaq · 2017-06-06T14:55:46Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+    private final ResponseProcessor next;
+    private final QuerySigningService<Long> querySigningService;
+    private final @NotNull TupleDataCache<String, Long, String> dataCache;
+    private final JsonNode respnoseCache;


s/respnose/response

cdeszaq · 2017-06-06T14:55:56Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+    private final ObjectWriter writer;
+    private final ResponseProcessor next;
+    private final QuerySigningService<Long> querySigningService;
+    private final @NotNull TupleDataCache<String, Long, String> dataCache;


Put this annotation on the constructor parameter

cdeszaq · 2017-06-06T14:59:21Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+        }
+
+        int statusCode = json.get(DruidJsonResponseContentKeys.STATUS_CODE.getName()).asInt();
+        if (statusCode == Status.NOT_MODIFIED.getStatusCode()) {


Could we get some more comments in here around what's supposed to be happening? I'd much rather follow comments for understanding than having to trace code closely to understand it.

cdeszaq · 2017-06-06T16:21:21Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+            try {
+                ((ObjectNode) json).set(
+                        DruidJsonResponseContentKeys.RESPONSE.getName(),
+                        respnoseCache == null


Won't this always be null? Perhaps I'm missing something, but why are we bothering with a conditional? Or perhaps the constructor was supposed to allow this to be set?

Nice catch!

cdeszaq · 2017-06-06T16:27:32Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+
+            try {
+                dataCache.set(
+                        DruidJsonResponseContentKeys.RESPONSE.getName(),


Using "response" as the cache key seems like a horrible idea... every query is going to overwrite this. Why not use the Druid query, like the V2 processor does?

cdeszaq · 2017-06-06T16:57:43Z

...roovy/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessorSpec.groovy

+                String.format(
+                        '{"%s": %d}',
+                        DruidJsonResponseContentKeys.STATUS_CODE.getName(),
+                        NOT_MODIFIED.getStatusCode()


Use Groovy's string interpolation here, it's much cleaner:

"""{"${DruidJsonResponseContentKeys.STATUS_CODE.name}": $NOT_MODIFIED.statusCode}"""

cdeszaq · 2017-06-06T16:57:55Z

...roovy/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessorSpec.groovy

+        etagCacheResponseProcessor.processResponse(json, druidAggregationQuery, Mock(LoggingContext))
+
+        then:
+        then:


2 then: tags?

cdeszaq · 2017-06-06T16:58:11Z

...roovy/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessorSpec.groovy

+                String.format(
+                        '{"%s": %d}',
+                        DruidJsonResponseContentKeys.STATUS_CODE.getName(),
+                        OK.getStatusCode()


Use groovy string templating

cdeszaq · 2017-06-06T17:01:41Z

...roovy/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessorSpec.groovy

+        given:
+        querySigningService.getSegmentSetId(_ as DruidAggregationQuery) >> Optional.empty()
+        JsonNode json = MAPPER.readTree(
+                String.format(


Use string templates

cdeszaq · 2017-06-06T17:07:10Z

...roovy/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessorSpec.groovy

+
+        then:
+        1 * dataCache.set(DruidJsonResponseContentKeys.RESPONSE.getName(), null, 'null')
+        1 * dataCache.set(DruidJsonResponseContentKeys.ETAG.getName(), null, '"someEtag"')


This looks wrong... are we really setting the cache value to "someEtag"? (ie. including the quotes around the string)

Yeah. I have removed it. Thanks!

garyluoex

Ask me if you have questions or doubt.

garyluoex · 2017-06-06T17:11:36Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+    private final @NotNull TupleDataCache<String, Long, String> dataCache;
+    private final JsonNode respnoseCache;
+
+


garyluoex · 2017-06-06T17:33:48Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+    public EtagCacheResponseProcessor(
+            ResponseProcessor next,
+            TupleDataCache<String, Long, String> dataCache,
+            QuerySigningService<Long> querySigningService,


We should not need this anymore, since we have etag now.

garyluoex · 2017-06-06T17:34:20Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+    private final ObjectWriter writer;
+    private final ResponseProcessor next;
+    private final QuerySigningService<Long> querySigningService;
+    private final @NotNull TupleDataCache<String, Long, String> dataCache;


I think we need a implementation of this as TupleDataCache<String, String, String>

Right. I've changed it.

garyluoex · 2017-06-06T17:41:51Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+    private final ResponseProcessor next;
+    private final QuerySigningService<Long> querySigningService;
+    private final @NotNull TupleDataCache<String, Long, String> dataCache;
+    private final JsonNode respnoseCache;


typo respnose -> response

garyluoex · 2017-06-06T17:42:58Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                                ? mapper.readTree(
+                                        dataCache.getDataValue(DruidJsonResponseContentKeys.RESPONSE.getName())
+                        )
+                                : respnoseCache


What is this respnoseCache? This thing will always be null right? It never got assigned from what I can see.

Nice catch!

garyluoex · 2017-06-06T17:51:56Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+            // make sure JSON response comes with etag
+            if (!json.has(DruidJsonResponseContentKeys.ETAG.getName())) {
+                logAndGetErrorCallback(ErrorMessageFormat.ETAG_MISSING_FROM_RESPONSE.format(), druidQuery);
+                return;


maybe the better behavior is to log a warning and simply move on without caching? @michael-mclawhorn @cdeszaq

Yes, cache should be best-effort. Log a warning and move on is the right thing to do. Not being able to safely cache doesn't prevent the response from being correctly returned.

garyluoex · 2017-06-06T17:54:32Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                        querySigningService.getSegmentSetId(druidQuery).orElse(null),
+                        writer.writeValueAsString(json.get(DruidJsonResponseContentKeys.RESPONSE.getName()))
+                );
+                dataCache.set(


There should only be one dataCache set only.

dataCache.set( cacheKey, etag, writer.writeValueAsString(json.get(DruidJsonResponseContentKeys.RESPONSE.getName())) );

cache key should be passed in to the class as constructor argument.

michael-mclawhorn · 2017-06-06T21:13:00Z

It looks like Gary caught most of my pending comments already and you've already fixed them. I think I'm about to 👍

QubitPi · 2017-06-06T21:14:15Z

@michael-mclawhorn :)

garyluoex · 2017-06-07T18:06:40Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                        : responseCache;
+                ((ObjectNode) json).set(
+                        DruidJsonResponseContentKeys.RESPONSE.getName(),
+                        responseCache


I still don't see what is the meaning of responseCache. This code is going to be only touched once so responseCache is going to be null and get assigned and never been used again outside?

garyluoex

Last comment most likely

garyluoex · 2017-06-07T20:17:45Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+        } else if (statusCode == Status.OK.getStatusCode()) { // If response is a OK, cache it, including etag
+            // make sure JSON response comes with etag
+            if (!json.has(DruidJsonResponseContentKeys.ETAG.getName())) {
+                logAndGetErrorCallback(ErrorMessageFormat.ETAG_MISSING_FROM_RESPONSE.format(), druidQuery);


dont throw error as commented before, instead LOG.warn and skip dataCache update and directly falls to next responseProcessor

garyluoex · 2017-06-07T20:18:03Z

...roovy/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessorSpec.groovy

+        json.get(DruidJsonResponseContentKeys.RESPONSE.getName()) == MAPPER.readTree('[{"k1":"v1"}]')
+    }
+
+    def "processResponse reports error when status code is OK but etag is missing and processResponse moves on without caching"() {


change behavior accordingly

michael-mclawhorn · 2017-06-14T16:24:47Z

You can make codacy happy if you use fewer raw exception types and instead use a mix of illegal state or illegal argument as appropriate.

michael-mclawhorn · 2017-06-14T16:26:39Z

@cdeszaq I'm going to clear your review if you don't have time to rereview in the near future.

garyluoex · 2017-06-15T19:06:36Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                        mapper.readTree(dataCache.getDataValue(cacheKey))
+                );
+            } catch (IOException ioe) {
+                throw new IllegalStateException(ioe);


I think we should use the logAndGetErrorCallback?

garyluoex · 2017-06-16T19:47:27Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                } catch (JsonProcessingException exception) {
+                    String message = "Unable to parse JSON response while caching";
+                    LOG.error(message, exception);
+                    throw new RuntimeException(message, exception);


I think we should use the logAndGetErrorCallback?

garyluoex · 2017-06-16T19:56:32Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+        // response processor
+        if (statusCode == Status.NOT_MODIFIED.getStatusCode()) {
+            try {
+                ((ObjectNode) json).set(


Can we make a copy of the json? It feels a bit dangerous modifying the json we get by passing in.

I can make a copy, except that it makes it impossible for test to check what processed json looks like

garyluoex · 2017-06-16T20:02:33Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                try {
+                    dataCache.set(
+                            cacheKey,
+                            json.get(DruidJsonResponseContentKeys.ETAG.getName()).asText(),


There is a bug in the prepare for cache pr that is merged. here, we need to fix it in this pr. The way to fix it is to add a additional key and use the new key where appropriate. Refer to the RFC for the key we need.

garyluoex

Last few things, otherwise good.

garyluoex · 2017-06-19T22:47:08Z

...roovy/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessorSpec.groovy

+        json.get(DruidJsonResponseContentKeys.RESPONSE.getName()) == MAPPER.readTree('[{"k1":"v1"}]')
+    }
+
+    def "processResponse reports error when status code is OK but etag is missing and processResponse moves on without caching"() {


Where does this test check error is reported?

I forgot to update test name. It keeps going without cache without reporting error

garyluoex · 2017-06-19T22:57:05Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                    );
+                } catch (JsonProcessingException exception) {
+                    logAndGetErrorCallback("Unable to parse JSON response while caching", druidQuery);
+                    return;


garyluoex · 2017-06-19T22:57:13Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+                );
+            } catch (IOException ioe) {
+                logAndGetErrorCallback(ioe.getLocalizedMessage(), druidQuery);
+                return;


garyluoex · 2017-06-19T22:57:18Z

...c/main/java/com/yahoo/bard/webservice/web/responseprocessors/EtagCacheResponseProcessor.java

+        // make sure JSON response comes with status code
+        if (!json.has(DruidJsonResponseContentKeys.STATUS_CODE.getName())) {
+            logAndGetErrorCallback(ErrorMessageFormat.STATUS_CODE_MISSING_FROM_RESPONSE.format(), druidQuery);
+            return;


Well, if you assume logAndGetErrorCallback does return, it's actually not the case.

2 approvals already; the last reviewer probably don't have time

QubitPi force-pushed the implement-EtagCacheResponseProcessor branch from 1d5f943 to 41dd73d Compare May 31, 2017 17:51

QubitPi added NEED 2 REVIEWS REVIEWABLE labels May 31, 2017

michael-mclawhorn requested changes Jun 2, 2017

View reviewed changes

cdeszaq previously requested changes Jun 6, 2017

View reviewed changes

garyluoex requested changes Jun 6, 2017

View reviewed changes

michael-mclawhorn approved these changes Jun 6, 2017

View reviewed changes

QubitPi added NEED 1 REVIEW and removed NEED 2 REVIEWS labels Jun 6, 2017

garyluoex requested changes Jun 7, 2017

View reviewed changes

QubitPi force-pushed the implement-EtagCacheResponseProcessor branch from 350efed to 1b85fea Compare June 7, 2017 22:08

garyluoex changed the base branch from master to prepare-for-cache-v3 June 15, 2017 18:15

QubitPi force-pushed the prepare-for-cache-v3 branch from 8efbe6a to 0f3977e Compare June 16, 2017 17:30

QubitPi added 11 commits June 16, 2017 12:09

Implement EtagCacheResponseProcessor

9f1ce02

Update CHANGELOG

cc3bfb3

Address @michael-mclawhorn's comments

23dd912

Assign cache to a variable

64c3d22

Change test name

48a410a

Address comments

4c8a35a

Fix indentation

43593a5

Remove extra empty line

9a38b86

Use exception in catch block

37e968a

Address @garyluoex's comments

3f3b3bd

Address @garyluoex's comments

dc8e413

QubitPi changed the base branch from prepare-for-cache-v3 to master June 16, 2017 19:11

QubitPi force-pushed the implement-EtagCacheResponseProcessor branch from 1b85fea to dc8e413 Compare June 16, 2017 19:11

yahoo deleted a comment Jun 16, 2017

garyluoex requested changes Jun 16, 2017

View reviewed changes

Address @garyluoex's comments

59899a8

yahoo deleted a comment Jun 17, 2017

garyluoex approved these changes Jun 20, 2017

View reviewed changes

Address @garyluoex's comments

d5b73c3

yahoo deleted a comment Jun 20, 2017

QubitPi added MERGEABLE and removed NEED 1 REVIEW REVIEWABLE labels Jun 20, 2017

QubitPi merged commit 0057f65 into master Jun 20, 2017

QubitPi deleted the implement-EtagCacheResponseProcessor branch July 12, 2017 00:30

		private final @NotNull TupleDataCache<String, Long, String> dataCache;
		private final JsonNode respnoseCache;

Implement EtagCacheResponseProcessor #311

Implement EtagCacheResponseProcessor #311

Conversation

QubitPi commented May 31, 2017 • edited Loading

michael-mclawhorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QubitPi Jun 2, 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

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

cdeszaq Jun 6, 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 Jun 6, 2017

QubitPi commented Jun 6, 2017

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

michael-mclawhorn commented Jun 14, 2017

michael-mclawhorn commented Jun 14, 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

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

QubitPi commented May 31, 2017 •

edited

Loading

QubitPi Jun 2, 2017 •

edited

Loading

cdeszaq Jun 6, 2017 •

edited

Loading