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

The cache key for /warm_up_cache/ differs from /explore_json/ #3840

Closed
3 tasks done
john-bodley opened this issue Nov 12, 2017 · 6 comments
Closed
3 tasks done

The cache key for /warm_up_cache/ differs from /explore_json/ #3840

john-bodley opened this issue Nov 12, 2017 · 6 comments

Comments

@john-bodley
Copy link
Member

john-bodley commented Nov 12, 2017

Make sure these boxes are checked before submitting your issue - thank you!

  • I have checked the superset logs for python stacktraces and included it here as text if any
  • I have reproduced the issue with at least the latest released version of superset
  • I have checked the issue tracker for the same issue and I haven't found one similar

Superset version

0.21.0dev

Expected results

Both the /warm_up_cache/ and /explore_json/ API endpoints should produce the same cache key for a given slice. A hashed version of form-data is used for the cache key, though it seems the AJAX form-data is mutated via i) the JavaScript and ii) via the Superset.explore_json(...) method thus the keys are not equivalent.

Actual results

Using the example dataset, the /warm_up_cache/?slice_id=60 endpoint hashes the following string for the cache key:

[
    (u'compare_lag', u'10'),
    (u'compare_suffix', u'o10Y'), 
    (u'granularity', u'ds'), 
    (u'groupby', []), 
    (u'json', u'false'), 
    (u'limit', u'100'), 
    (u'markup_type', u'markdown'), 
    (u'metric', u'sum__num'), 
    (u'metrics', [u'sum__num']), 
    (u'rotation', u'square'), 
    (u'row_limit', 50000), 
    (u'series', u'name'), 
    (u'since', u'100 years ago'),
    (u'size_from', u'10'),
    (u'size_to', u'70'), 
    (u'slice_id', 60), 
    (u'slice_name', u'Name Cloud'), 
    (u'until', u'now'), 
    (u'viz_type', u'word_cloud'), 
    (u'where', u'')
]

whereas /slice/60/ endpoint hashes the following string for the cache key:

[
    (u'color_scheme', u'bnbColors'), 
    (u'datasource', u'3__table'), 
    (u'filters', []), 
    (u'granularity_sqla', u'ds'), 
    (u'having', u''), 
    (u'limit', u'100'), 
    (u'metric', u'sum__num'), 
    (u'rotation', u'square'),
    (u'series', u'name'), 
    (u'since', u'100 years ago'), 
    (u'size_from', u'10'), 
    (u'size_to', u'70'), 
    (u'slice_id', 60), 
    (u'time_grain_sqla', u'Time Column'), 
    (u'until', u'now'), 
    (u'viz_type', u'word_cloud'), 
    (u'where', u'')
]

I'm not sure what the correct way to proceed is. I presume there are a couple of options:

  1. Update the cache key logic to contain a whitelist of form-data fields to hash. Note this may be difficult to maintain. In essence differentiate between form-data elements which are related to the payload and those which are not.
  2. Deprecate the /warm_up_cache/ API endpoint and augment existing non-API endpoints to pass the force request argument, i.e., /slice/60/?force=true. Note the issue with this approach is one may need to use a headless browser if the intent is to systematically warm up the cache.

Steps to reproduce

See above.

@mistercrunch
Copy link
Member

I also noticed that in some cases the dashbboard view and the explore view somehow generate different URLs and cache keys.

@john-bodley
Copy link
Member Author

@mistercrunch would this PR not remedy the issue you've been noticing? #3809

@mistercrunch
Copy link
Member

Hoping so!

@john-bodley
Copy link
Member Author

@mistercrunch one solution would be to use the query string as the cache key as opposed to the form data (the query response is actually what we're trying to cache). This would resolve this issue and potentially reduce the number of keys.

Note that the cache key is now slice agnostic, so if multiple slices or explorer views use the same query, force refreshing of cache will refresh all dependents.

@ghulands
Copy link

@john-bodley can this be closed?

@ukm21
Copy link

ukm21 commented Jul 7, 2021

facing this issue in superset 1.1.0 where cache key is different for warm_up_cache and explore_chart

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants