Add config to ignore partial/volatile intervals and cache everything …

…in cache V2

CHANGELOG.md

@@ -128,6 +128,10 @@ Current
 
 ### Changed:
 
+- [Add config to ignore partial/volatile intervals and cache everything in cache V2](https://github.com/yahoo/fili/pull/645)
+    * In cache V2, user should be able to decide whether partial data or volatile data should be cached or not. This PR
+      adds a config that allows the user to do this.
+
 - [Lift required override on deprecated method in MetricLoader](https://github.com/yahoo/fili/pull/609)
     * Add default implementation to deprecated `loadMetricDictionary` in `MetricLoader` so that downstream projects are
       able to implement the new version without worrying about the deprecated version.

fili-core/src/main/java/com/yahoo/bard/webservice/config/BardFeatureFlag.java

@@ -10,6 +10,7 @@ public enum BardFeatureFlag implements FeatureFlag {
     @Deprecated DRUID_CACHE("druid_cache_enabled"),
     @Deprecated DRUID_CACHE_V2("druid_cache_v2_enabled"),
     QUERY_SPLIT("query_split_enabled"),
+    CACHE_PARTIAL_DATA("cache_partial_data"),
     TOP_N("top_n_enabled"),
     DATA_FILTER_SUBSTRING_OPERATIONS("data_filter_substring_operations_enabled"),
     INTERSECTION_REPORTING("intersection_reporting_enabled"),

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

@@ -2,6 +2,7 @@
 // Licensed under the terms of the Apache license. Please see LICENSE.md file distributed with this work for terms.
 package com.yahoo.bard.webservice.web.responseprocessors;
 
+import static com.yahoo.bard.webservice.config.BardFeatureFlag.CACHE_PARTIAL_DATA;
 import static com.yahoo.bard.webservice.web.handlers.PartialDataRequestHandler.getPartialIntervalsWithDefault;
 import static com.yahoo.bard.webservice.web.handlers.VolatileDataRequestHandler.getVolatileIntervalsWithDefault;
 
@@ -85,7 +86,7 @@ public HttpErrorCallback getErrorCallback(DruidAggregationQuery<?> druidQuery) {
 
     @Override
     public void processResponse(JsonNode json, DruidAggregationQuery<?> druidQuery, LoggingContext metadata) {
-        if (isCacheable()) {
+        if (!CACHE_PARTIAL_DATA.isOn() || isCacheable()) {
             String valueString = null;
             try {
                 valueString = writer.writeValueAsString(json);
@@ -112,6 +113,7 @@ public void processResponse(JsonNode json, DruidAggregationQuery<?> druidQuery,
                 );
             }
         }
+
         next.processResponse(json, druidQuery, metadata);
     }
 

fili-core/src/test/groovy/com/yahoo/bard/webservice/config/FeatureFlagRegistrySpec.groovy

@@ -36,9 +36,10 @@ class FeatureFlagRegistrySpec extends Specification {
 
         then:
         values == ["partial_data_enabled", "druid_cache_enabled", "druid_cache_v2_enabled", "query_split_enabled",
-                   "top_n_enabled", "data_filter_substring_operations_enabled", "intersection_reporting_enabled",
-                   "updated_metadata_collection_names_enabled", "druid_coordinator_metadata_enabled",
-                   "druid_lookup_metadata_enabled", "druid_dimensions_loader_enabled", "case_sensitive_keys_enabled"] as Set
+                   "cache_partial_data", "top_n_enabled", "data_filter_substring_operations_enabled",
+                   "intersection_reporting_enabled", "updated_metadata_collection_names_enabled",
+                   "druid_coordinator_metadata_enabled", "druid_lookup_metadata_enabled",
+                   "druid_dimensions_loader_enabled", "case_sensitive_keys_enabled"] as Set
     }
 
     @Unroll

fili-core/src/test/groovy/com/yahoo/bard/webservice/web/responseprocessors/CacheV2ResponseProcessorSpec.groovy

@@ -3,6 +3,7 @@
 package com.yahoo.bard.webservice.web.responseprocessors
 
 import static com.yahoo.bard.webservice.async.ResponseContextUtils.createResponseContext
+import static com.yahoo.bard.webservice.config.BardFeatureFlag.CACHE_PARTIAL_DATA
 import static com.yahoo.bard.webservice.web.responseprocessors.ResponseContextKeys.MISSING_INTERVALS_CONTEXT_KEY
 import static com.yahoo.bard.webservice.web.responseprocessors.ResponseContextKeys.VOLATILE_INTERVALS_CONTEXT_KEY
 
@@ -55,10 +56,17 @@ class CacheV2ResponseProcessorSpec extends Specification {
 
     CacheV2ResponseProcessor crp
 
+    boolean cache_partial_data
+
     def setup() {
         querySigningService.getSegmentSetId(_) >> Optional.of(1234L)
         segmentId = querySigningService.getSegmentSetId(groupByQuery).get()
         crp = new CacheV2ResponseProcessor(next, cacheKey, dataCache, querySigningService, MAPPER)
+        cache_partial_data = CACHE_PARTIAL_DATA.isOn()
+    }
+
+    def cleanup() {
+        CACHE_PARTIAL_DATA.setOn(cache_partial_data)
     }
 
     def "Test Constructor"() {
@@ -105,6 +113,7 @@ class CacheV2ResponseProcessorSpec extends Specification {
 
     def "After error saving to cache, process response continues"() {
         when:
+        CACHE_PARTIAL_DATA.setOn(true)
         crp.processResponse(json, groupByQuery, null)
 
         then:
@@ -113,6 +122,17 @@ class CacheV2ResponseProcessorSpec extends Specification {
         1 * dataCache.set(cacheKey, segmentId, '[]') >> { throw new IllegalStateException() }
     }
 
+    def "After error is not saved to cache, process response continues"() {
+        when:
+        CACHE_PARTIAL_DATA.setOn(false)
+        crp.processResponse(json, groupByQuery, null)
+
+        then:
+        0 * next.getResponseContext() >> responseContext
+        1 * next.processResponse(json, groupByQuery, null)
+        1 * dataCache.set(cacheKey, segmentId, '[]') >> { throw new IllegalStateException() }
+    }
+
     def "After json serialization error of the cache value, process response continues"() {
         setup:
         ObjectMapper localMapper = Mock(ObjectMapper)
@@ -131,30 +151,35 @@ class CacheV2ResponseProcessorSpec extends Specification {
         0 * dataCache.set(*_)
     }
 
-    def "Partial data doesn't cache and then continues"() {
-        setup:
-        ResponseContext responseContext = createResponseContext([(MISSING_INTERVALS_CONTEXT_KEY.name) : nonEmptyIntervals])
+    @Unroll
+    def "When cache_partial_data is turned #on, #typed data with #simplifiedIntervalList #is cached and then continues"() {
+        setup: "turn on or off cache_partial_data"
+        CACHE_PARTIAL_DATA.setOn(cachePartialData)
 
-        when:
+        when: "we process respnse"
         crp.processResponse(json, groupByQuery, null)
 
-        then:
-        2 * next.getResponseContext() >> responseContext
+        then: "data cache is handled property and continues"
+        numGetContext * next.getResponseContext() >> createResponseContext(
+                [(MISSING_INTERVALS_CONTEXT_KEY.name): simplifiedIntervalList]
+        )
+        numCache * dataCache.set(*_)
         1 * next.processResponse(json, groupByQuery, null)
-        0 * dataCache.set(*_)
-    }
-
-    def "Volatile data doesn't cache and then continues"() {
-        setup:
-        ResponseContext responseContext = createResponseContext([(VOLATILE_INTERVALS_CONTEXT_KEY.name) : nonEmptyIntervals])
-
-        when:
-        crp.processResponse(json, groupByQuery, null)
 
-        then:
-        2 * next.getResponseContext() >> responseContext
-        1 * next.processResponse(json, groupByQuery, null)
-        0 * dataCache.set(*_)
+        where:
+        cachePartialData | typed      | simplifiedIntervalList
+        false            | "partial"  | intervals
+        true             | "partial"  | nonEmptyIntervals
+        true             | "partial"  | intervals
+        false            | "volatile" | intervals
+        true             | "volatile" | nonEmptyIntervals
+        true             | "volatile" | intervals
+
+        on = cachePartialData ? "on" : "off"
+        is = simplifiedIntervalList.empty ? "is" : "is not"
+
+        numGetContext = cachePartialData ? 2 : 0
+        numCache = simplifiedIntervalList.empty ? 1 : 0
     }
 
     def "Overly long data doesn't cache and then continues"() {