You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2020/10/02 00:25:18 UTC

[GitHub] [incubator-superset] ktmud opened a new pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

ktmud opened a new pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137


   ### SUMMARY
   
   This PR fixes a bug in #7032 and #10963 where different ETags are generated for requests to different running Superset instances even when the underlying data are the same. Currently new ETags will be generated every time Flask is restarted or when there are multiple WSGI workers running at the same time (either via gunicorn or some other tools).
   
   Also added `cache-control: no-cache` for dashboards so that the browser always asks the server to validate data freshness, instead of serving disk cache.
   
   #### Problem
   
   When running Superset behind Gunicorn with multiple workers, each worker instance generates different etags in `etag_cache`, making the ETags much less useful because users would often hit a different worker on each request, and constantly invalidate the previous Etags from other workers.
   
   This is because `flask_caching` memoization does not work well with [class instance methods](https://github.com/apache/incubator-superset/blob/5f145ac2aab74b3c9f00d97800aaa5899678a499/superset/views/core.py#L517-L520). 
   
   In `_memoize_make_cache_key`, [positional arguments are used to generate cache keys](https://github.com/sh4nks/flask-caching/blob/e81c0b9c11ab66e3ab05538f36417a64f2b37a25/flask_caching/__init__.py#L650-L657
   ), but for a class instance method, the first argument will always be `self`, which by default will be formatted to a unique string with memory locations. E.g.
   
   ```
   <superset.views.core.Superset object at 0x7ffb67416a50>
   ```
   
   #### Solution
   
   Give `superset.views.core.Superset` a determinate `__repr__` such that the cache keys generated by `flask_caching` can be more stable. I'm using app version string since it makes sense to invalidate the cache every time Superset is upgraded.
   
   So instead of generating the following cache key update string [here]([here](https://github.com/sh4nks/flask-caching/blob/v1.9.0/flask_caching/__init__.py#L657)):
   
   ```
   superset.views.core.Superset.dashboard('<superset.views.core.Superset object at 0x7ffb67416a50>', '15634')OrderedDict()
   ```
   
   it will generates something like this:
   
   ```
   superset.views.core.Superset.dashboard('Superset.views.core.Superset@v0.999.0dev', '15634')OrderedDict()
   ```
   
   ### BEFORE/AFTER SCREENSHOTS
   
   #### Before
   
   Second visit to a dashboard page uses disk cache if not a refresh:
   
   <img src="https://user-images.githubusercontent.com/335541/94875266-2a8c8100-0409-11eb-80a6-8770b2334e1d.png" width="450" />
   
   A refresh request (almost) always returns 200.
   
   #### After
   
   Second and subsequent visits to a dashboard page (regardless whether is a refresh or not) correctly make a roundtrip request to the server and the server correctly returns HTTP 304 (not modified).
   
   <img src="https://user-images.githubusercontent.com/335541/94875844-c4086280-040a-11eb-8c50-aa647ccf82d1.png" width="450" />
   
   ### TEST PLAN
   
   1. Start Superset with gunicorn and multiple workers
      ```
       gunicorn \
         --bind 0.0.0.0:8089 \
         --workers 5 \
         "superset.app:create_app()"
      ```
   2. Visit the dashboard page and refresh the page multiple times. DO NOT disable browser cache.
   3. Previously you will almost always see HTTP 200 responses, with this fix, you should see HTTP 304 since the second request and most subsequent page loads are going to be much faster.
   
   Not sure `__repr__` is used in other places, but I doubt they will require memory locations and version string is not enough.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#issuecomment-702874881


   For dashboard requests:
   We set header **Cache-Control: no-cache**
   If you enable dashboard etag header feature, dashboard metadata will be stored in browser cache. But for every new dashboard request, browser still sends out request to server-side to validate if cache is still valid, to make sure no other users changed dashboard since last visit. If server-side response 304, browser will use cached data. otherwise, server-side will response 200 with newer version of dashboard metadata. so this feature only save server-side processing time but still need a roundtrip.
   
   For explore_json requests:
   We set header **Cache-Control: public**
   If you use GET request for explore_json, query results will be cached in browser, and browser will not send requests to server-side until the cache expired (set by Expire header). This is current behavior (per #7032), this PR and #10963 will not change it.
   
   cc @betodealmeida and @bkyryliuk since you are interested in this topic.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] ktmud commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498951863



##########
File path: superset/views/core.py
##########
@@ -1599,12 +1606,13 @@ def publish(  # pylint: disable=no-self-use
 
     @has_access
     @etag_cache(
-        0,
+        CACHE_DEFAULT_TIMEOUT,

Review comment:
       OK, changed it back to `0`.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498918954



##########
File path: superset/views/core.py
##########
@@ -1599,12 +1606,13 @@ def publish(  # pylint: disable=no-self-use
 
     @has_access
     @etag_cache(
-        0,
+        CACHE_DEFAULT_TIMEOUT,

Review comment:
       for Dashboard's etag, it will prefer to be **force** **expired** in far future, since cache will be validated per user request (with dashboard's last modified_on). `CACHE_DEFAULT_TIMEOUT` may used by the Superset system for other purpose, for example, backend query results cache age, it probably only 1 day or 36 hours.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] ktmud commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498952290



##########
File path: superset/views/core.py
##########
@@ -171,6 +171,13 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
 
     logger = logging.getLogger(__name__)
 
+    def __repr__(self):

Review comment:
       I can't think of a scenario where this will cause security issues. But I guess it does make it easier to invalidate cache if we encode it with some shared secret.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] graceguo-supercat commented on pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#issuecomment-702874881


   For dashboard requests:
   If you enable dashboard etag header feature, dashboard metadata will be stored in browser cache. But for every dashboard request, browser still send out request to server-side to validate if cache is still valid, to make sure dashboard metadata is in-sync. If server-side response 304, browser will use cached data. otherwise, server-side will response 200 with newer version of dashboard metadata. so this feature only save server-side processing time but still need a roundtrip.
   
   For explore_json requests:
   If you use GET request for explore_json, query results will be cached in browser, and browser will not send requests to server-side until the cache expired (set by Expire header). This is current behavior (per #7032), this PR and #10963 will not change it.
   
   cc @betodealmeida and @bkyryliuk since you are interested in this topic.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] ktmud commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498579556



##########
File path: superset/utils/decorators.py
##########
@@ -51,6 +51,7 @@ def etag_cache(
     check_perms: Callable[..., Any],
     get_last_modified: Optional[Callable[..., Any]] = None,
     skip: Optional[Callable[..., Any]] = None,
+    must_revalidate: Optional[bool] = False,

Review comment:
       We could potentially always assume `must_revalidate` if `get_last_modified` is provided. Because validation only makes sense when there is a way to programmatically get a resource's the "real" last modified time.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] graceguo-supercat merged pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#issuecomment-702874881


   For dashboard requests:
   If you enable dashboard etag header feature, dashboard metadata will be stored in browser cache. But for every new dashboard request, browser still sends out request to server-side to validate if cache is still valid, to make sure no other users changed dashboard since last visit. If server-side response 304, browser will use cached data. otherwise, server-side will response 200 with newer version of dashboard metadata. so this feature only save server-side processing time but still need a roundtrip.
   
   For explore_json requests:
   If you use GET request for explore_json, query results will be cached in browser, and browser will not send requests to server-side until the cache expired (set by Expire header). This is current behavior (per #7032), this PR and #10963 will not change it.
   
   cc @betodealmeida and @bkyryliuk since you are interested in this topic.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] etr2460 commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
etr2460 commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498595090



##########
File path: superset/views/core.py
##########
@@ -171,6 +171,13 @@ class Superset(BaseSupersetView):  # pylint: disable=too-many-public-methods
 
     logger = logging.getLogger(__name__)
 
+    def __repr__(self):

Review comment:
       do you think it's safe for this to be guessable? Or should we encode it with the shared secret instead? 
   
   I don't really know, but throwing it out there




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] graceguo-supercat edited a comment on pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#issuecomment-702874881


   For dashboard requests:
   If you enable dashboard etag header feature, dashboard metadata will be stored in browser cache. But for every new dashboard request, browser still sends out request to server-side to validate if cache is still valid, to make sure dashboard metadata is in-sync. If server-side response 304, browser will use cached data. otherwise, server-side will response 200 with newer version of dashboard metadata. so this feature only save server-side processing time but still need a roundtrip.
   
   For explore_json requests:
   If you use GET request for explore_json, query results will be cached in browser, and browser will not send requests to server-side until the cache expired (set by Expire header). This is current behavior (per #7032), this PR and #10963 will not change it.
   
   cc @betodealmeida and @bkyryliuk since you are interested in this topic.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] ktmud commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498579815



##########
File path: superset/views/core.py
##########
@@ -1599,12 +1606,13 @@ def publish(  # pylint: disable=no-self-use
 
     @has_access
     @etag_cache(
-        0,
+        CACHE_DEFAULT_TIMEOUT,

Review comment:
       Setting `max_age` to `0` will set cache expiration to a far future date (1 year from now), which is unlikely to be what we actually need unless we are serving static resources.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] codecov-commenter commented on pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#issuecomment-702482343


   # [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11137?src=pr&el=h1) Report
   > Merging [#11137](https://codecov.io/gh/apache/incubator-superset/pull/11137?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/7a72082d31257012235c962db853e908f8e4af4c?el=desc) will **decrease** coverage by `4.22%`.
   > The diff coverage is `14.28%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/incubator-superset/pull/11137/graphs/tree.svg?width=650&height=150&src=pr&token=KsB0fHcx6l)](https://codecov.io/gh/apache/incubator-superset/pull/11137?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master   #11137      +/-   ##
   ==========================================
   - Coverage   65.86%   61.63%   -4.23%     
   ==========================================
     Files         826      826              
     Lines       38976    39017      +41     
     Branches     3669     3667       -2     
   ==========================================
   - Hits        25671    24050    -1621     
   - Misses      13195    14786    +1591     
   - Partials      110      181      +71     
   ```
   
   | Flag | Coverage Δ | |
   |---|---|---|
   | #cypress | `?` | |
   | #javascript | `62.21% <ø> (+<0.01%)` | :arrow_up: |
   | #python | `61.29% <14.28%> (-0.02%)` | :arrow_down: |
   
   Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.
   
   | [Impacted Files](https://codecov.io/gh/apache/incubator-superset/pull/11137?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [superset/utils/decorators.py](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `49.25% <0.00%> (-1.52%)` | :arrow_down: |
   | [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.01% <50.00%> (-0.29%)` | :arrow_down: |
   | [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL1NxbExhYi9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvQXBwLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9BcHAuanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/explore/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvaW5kZXguanN4) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/dashboard/index.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9pbmRleC5qc3g=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupColors.js](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/chart/ChartContainer.jsx](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2NoYXJ0L0NoYXJ0Q29udGFpbmVyLmpzeA==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | [superset-frontend/src/setup/setupFormatters.js](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRm9ybWF0dGVycy5qcw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
   | ... and [183 more](https://codecov.io/gh/apache/incubator-superset/pull/11137/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11137?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/11137?src=pr&el=footer). Last update [7a72082...66b9556](https://codecov.io/gh/apache/incubator-superset/pull/11137?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] ktmud commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498579815



##########
File path: superset/views/core.py
##########
@@ -1599,12 +1606,13 @@ def publish(  # pylint: disable=no-self-use
 
     @has_access
     @etag_cache(
-        0,
+        CACHE_DEFAULT_TIMEOUT,

Review comment:
       Setting `max_age` will just set cache expiration to a far future date (1 year from now), which is almost never what we want unless we are serving static resources.

##########
File path: superset/views/core.py
##########
@@ -1599,12 +1606,13 @@ def publish(  # pylint: disable=no-self-use
 
     @has_access
     @etag_cache(
-        0,
+        CACHE_DEFAULT_TIMEOUT,

Review comment:
       Setting `max_age` to `0` will set cache expiration to a far future date (1 year from now), which is almost never what we want unless we are serving static resources.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org


[GitHub] [incubator-superset] graceguo-supercat commented on a change in pull request #11137: fix: enable consistent etag across workers and force no-cache for dashboards

Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #11137:
URL: https://github.com/apache/incubator-superset/pull/11137#discussion_r498918954



##########
File path: superset/views/core.py
##########
@@ -1599,12 +1606,13 @@ def publish(  # pylint: disable=no-self-use
 
     @has_access
     @etag_cache(
-        0,
+        CACHE_DEFAULT_TIMEOUT,

Review comment:
       for Dashboard's etag, it will prefer to be expire in far future, since cache will be validated per user request (with dashboard's last modified_on). `CACHE_DEFAULT_TIMEOUT` may used by the Superset system for other purpose, for example, backend query results cache age, it probably only 1 day or 36 hours.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org