Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config to ignore partial/volatile intervals and cache everything in cache V2 #645

Merged
merged 4 commits into from
Mar 9, 2018

Conversation

QubitPi
Copy link
Contributor

@QubitPi QubitPi commented Mar 9, 2018

No description provided.

@yahoo yahoo deleted a comment Mar 9, 2018
@QubitPi QubitPi changed the title Ignore partial/volatile intervals and cache everything in cache V2 Add config to ignore partial/volatile intervals and cache everything in cache V2 Mar 9, 2018
Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me.

@@ -10,6 +10,7 @@
@Deprecated DRUID_CACHE("druid_cache_enabled"),
@Deprecated DRUID_CACHE_V2("druid_cache_v2_enabled"),
QUERY_SPLIT("query_split_enabled"),
CACHEABLE_CHECK("cacheable_check"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename to 'CACHE_PARTIAL_DATA'

@michael-mclawhorn michael-mclawhorn dismissed their stale review March 9, 2018 18:57

I missed something

Copy link
Contributor

@michael-mclawhorn michael-mclawhorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs a README

@yahoo yahoo deleted a comment Mar 9, 2018
@@ -131,32 +151,96 @@ class CacheV2ResponseProcessorSpec extends Specification {
0 * dataCache.set(*_)
}

def "Partial data doesn't cache and then continues"() {
setup:
def "When cache_partial_data is turned off, partial data is cached and then continues"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can rewrite these 6 tests with @Unroll:

@Unroll
    def "When cache_partial_data is turned #isOn, #dataType is cached and then continues"() {
        setup: "check if volatile data can be cached"
        CACHE_PARTIAL_DATA.setOn(isOnBoolean)

        when: "we process response"
        crp.processResponse(json, groupByQuery, null)

        then: "query is checked and volatile data is cached"
        methodRunCounts[0] * next.getResponseContext() >> context
        methodRunCounts[1] * next.processResponse(json, groupByQuery, null)
        methodRunCounts[2] * dataCache.set(*_)

        where:
        isOn   | dataType       | isOnBoolean   | context                                                                           | methodRunCounts
        "off"  | "partial data" | false         | createResponseContext([(MISSING_INTERVALS_CONTEXT_KEY.name): intervals])          | [0,1,1]
        "on"   | "partial data" | true          | createResponseContext([(MISSING_INTERVALS_CONTEXT_KEY.name) : nonEmptyIntervals]) | [2,1,0]
        "on"   | "partial data" | true          | createResponseContext([:])                                                        | [2,1,1]
        "off"  | "volatile data"| false         | createResponseContext([(MISSING_INTERVALS_CONTEXT_KEY.name): intervals])          | [0,1,1]
        "on"   | "volatile data"| true          | createResponseContext([(VOLATILE_INTERVALS_CONTEXT_KEY.name) : nonEmptyIntervals])| [2,1,0]
        "on"   | "volatile data"| true          | createResponseContext([:])                                                        | [2,1,1]
    }

@QubitPi
Copy link
Contributor Author

QubitPi commented Mar 9, 2018

@michael-mclawhorn @tarrantzhang Thank you for the approvals!

@QubitPi QubitPi merged commit 180df63 into master Mar 9, 2018
@QubitPi QubitPi deleted the dont-check-cacheable branch March 9, 2018 22:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants