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/09/18 21:40:13 UTC
[GitHub] [incubator-superset] graceguo-supercat opened a new pull request #10963: [WIP]feat: enable eTag header for dashboard
graceguo-supercat opened a new pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TEST PLAN
<!--- What steps should be taken to verify the changes -->
### 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] bkyryliuk commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r494446189
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
+ """
+ Get latest changed datetime for a dashboard. The change could be dashboard
Review comment:
s/Get latest changed datetime for a dashboard.
/Get latest changed datetime for a dashboard and it's charts
----------------------------------------------------------------
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] bkyryliuk commented on pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698055692
> response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard' changed_on and its slices change
great thanks, it looks like it addresses question #1, what about 2 and 3?
e.g. for #2
* if chart was cached for ~10 hours and etag cached the dashboard, does it mean that chart on this dashboard will be cached for 10 hours + etag expiration time ?
for #3
* how it will affect dashboard / datasource changes like annotations, changed to the default filters, css rules, etc would those changes be cached for the user as well?
Sorry if those questions do not make sense or are obvious. I am just trying to understand the flow here
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495283966
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
+ """
+ Get latest changed datetime for a dashboard. The change could be dashboard
Review comment:
yes. I rename it to `get_dashboard_latest_changedon_dt`.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495292236
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
Review comment:
Please see other functions that used by decorator:
```This function takes `self` since it must have the same signature as the
the decorated method.```
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat merged pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963
----------------------------------------------------------------
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] bkyryliuk commented on pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-697977809
@graceguo-supercat can you please describe what effect this change will have on
1. permission checks, e.g, if user gained access, what they still see permission denied if that is cached
2. how it will interact with chart cache
3. how it will interact with datasource / dashboard changes
----------------------------------------------------------------
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] betodealmeida commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
betodealmeida commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496297046
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +94,26 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if get_latest_changed_on:
+ content_changed_time = get_latest_changed_on(*args, **kwargs)
+ if (
+ response
+ and response.last_modified
+ and response.last_modified.timestamp()
+ < content_changed_time.timestamp()
+ ):
+ response = None
+ else:
+ content_changed_time = datetime.utcnow()
Review comment:
I know generalizing too soon is not a good practice, but I wonder if we should pass a callable called `is_stale` here. It would simplify the decorator logic, and since it would be defined closer to the dashboard it might simplify the logic there as well.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495289551
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
Review comment:
this is a little confusing: Dashboard class has a get `datasources` function. So i use it in `check_dashboard_perms` function. But this function was to group slices into datasources, and the result will be used by another feature:
https://github.com/apache/incubator-superset/blob/ba009b7c09d49f2932fd10269882c901bc020c1d/superset/views/core.py#L1626
Instead of datasource.data, datasource.data_for_slices(slices) can reduce the initial dashboard data load size.
So right now i removed this helper function from utils, and build dict in the dashboard function. But i rename datasource to `slices_by_datasources` for clarification.
----------------------------------------------------------------
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 edited a comment on pull request #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=h1) Report
> Merging [#10963](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/dc893fe1b3b86e2921bb77e08a531db46b3f8e5d?el=desc) will **decrease** coverage by `0.72%`.
> The diff coverage is `71.42%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10963 +/- ##
==========================================
- Coverage 61.46% 60.73% -0.73%
==========================================
Files 382 382
Lines 24139 24154 +15
==========================================
- Hits 14836 14669 -167
- Misses 9303 9485 +182
```
| Flag | Coverage Δ | |
|---|---|---|
| #python | `60.73% <71.42%> (-0.73%)` | :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/10963?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/utils/decorators.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `53.52% <56.25%> (-1.48%)` | :arrow_down: |
| [superset/views/utils.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvdXRpbHMucHk=) | `83.07% <76.47%> (-1.24%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.46% <83.33%> (-0.01%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `65.62% <0.00%> (-9.38%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `82.14% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
| ... and [11 more](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?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/10963?src=pr&el=footer). Last update [dc893fe...bcf90b9](https://codecov.io/gh/apache/incubator-superset/pull/10963?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] bkyryliuk commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r494446189
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
+ """
+ Get latest changed datetime for a dashboard. The change could be dashboard
Review comment:
s/Get latest changed datetime for a dashboard.
/Get latest changed datetime for a dashboard and it's charts
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
Review comment:
looks like a good candidate for the Dashboard class method
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
Review comment:
what is _self here? ideally we should avoid Any types
##########
File path: superset/utils/decorators.py
##########
@@ -34,6 +34,10 @@
logger = logging.getLogger(__name__)
+def is_dashboard_request(kwargs: Any) -> bool:
+ return kwargs.get("dashboard_id_or_slug") is not None
Review comment:
doesn't seem robust, it it possible to validate via uri path or just pass a param ?
##########
File path: superset/views/utils.py
##########
@@ -490,6 +538,26 @@ def check_slice_perms(_self: Any, slice_id: int) -> None:
viz_obj.raise_for_access()
+def check_dashboard_perms(_self: Any, dashboard_id_or_slug: str) -> None:
Review comment:
best practice is to have a unit test for every function, it would be great if you could add some
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +103,25 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if check_latest_changed_on:
+ latest_changed_on = check_latest_changed_on(*args, **kwargs)
+ if response and response.last_modified:
+ latest_record = response.last_modified.replace(
+ tzinfo=timezone.utc
Review comment:
this assumes that superset server runs in utc zone, it may be safer to make it as a superset config variable
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698043672
`etag_cache` decorator works this way:
- explore_json flow:
1. http request come in
1. check_slice_perms
1. if method is POST, run query (not use cache). Otherwise check if this request has cache.
1. if no cached response, run query and create a cache key for the response
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is now (response time).
- dashboard flow:
1. http request come in
1. check_dashboard_perms
1. if method is POST, or feature is not enable, run dashboard function to build response (not use cache). Otherwise check if this request has cache.
1. if no cached response, run dashboard function.
1. if has cache, compare cached time with dashboard last modified time: it could be dashboard's changed_on > last cache_time or any slice's changed_on > last_cache_time. If cache is stale, run dashboard function.
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard and its slices)
----------------------------------------------------------------
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] bkyryliuk commented on pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698451673
> **Note:**
> For explore_json requests, we want to cache **query results**.
>
> For dashboard requests, we want to cache **dashboard bootstrap data**, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the **results itself are not in the dashboard bootstrap data**.
>
> This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
>
> #2
> if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. Since query results is not part of dashboard bootstrap data, it's not included in the dashboard cache either.
>
> #3:
> annotations: dashboard has no annotation. If slice's annotation was changed, slice entity's `params` and `changed_on` attribute will be changed.
> changed to the default filters, css rules, etc: these change will be stored in dashboard metadata, and will change dashboard `changed_on` attribute.
Big thanks for the explanation!
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495282712
##########
File path: superset/utils/decorators.py
##########
@@ -72,6 +80,12 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
if request.method == "POST":
return f(*args, **kwargs)
+ # if it is dashboard request but feature is not eabled,
+ # do not use cache
+ is_dashboard = is_dashboard_request(kwargs)
Review comment:
thanks! after introduce this `skip`, `dashboard_id` is not needed in decorator at all.
----------------------------------------------------------------
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 edited a comment on pull request #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=h1) Report
> Merging [#10963](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/e4e78b66fa82acbe8e5bc425d697ff11b4a0cc3f?el=desc) will **decrease** coverage by `0.12%`.
> The diff coverage is `70.17%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10963 +/- ##
==========================================
- Coverage 61.57% 61.44% -0.13%
==========================================
Files 815 382 -433
Lines 38337 24165 -14172
Branches 3599 0 -3599
==========================================
- Hits 23606 14849 -8757
+ Misses 14545 9316 -5229
+ Partials 186 0 -186
```
| Flag | Coverage Δ | |
|---|---|---|
| #javascript | `?` | |
| #python | `61.44% <70.17%> (-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/10963?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/config.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.42% <0.00%> (ø)` | |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `53.52% <56.25%> (-1.48%)` | :arrow_down: |
| [superset/views/utils.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvdXRpbHMucHk=) | `83.07% <76.47%> (-1.24%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.46% <83.33%> (-0.01%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `81.61% <0.00%> (-0.68%)` | :arrow_down: |
| [...et-frontend/src/explore/controlPanels/sections.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvY29udHJvbFBhbmVscy9zZWN0aW9ucy5qc3g=) | | |
| [...set-frontend/src/explore/actions/exploreActions.js](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvYWN0aW9ucy9leHBsb3JlQWN0aW9ucy5qcw==) | | |
| [superset-frontend/src/profile/App.tsx](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3Byb2ZpbGUvQXBwLnRzeA==) | | |
| [...dashboard/components/gridComponents/new/NewRow.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb21wb25lbnRzL2dyaWRDb21wb25lbnRzL25ldy9OZXdSb3cuanN4) | | |
| [...ntend/src/dashboard/containers/DashboardHeader.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2Rhc2hib2FyZC9jb250YWluZXJzL0Rhc2hib2FyZEhlYWRlci5qc3g=) | | |
| ... and [428 more](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?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/10963?src=pr&el=footer). Last update [e4e78b6...fcc1868](https://codecov.io/gh/apache/incubator-superset/pull/10963?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] graceguo-supercat commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495297257
##########
File path: superset/views/utils.py
##########
@@ -490,6 +538,26 @@ def check_slice_perms(_self: Any, slice_id: int) -> None:
viz_obj.raise_for_access()
+def check_dashboard_perms(_self: Any, dashboard_id_or_slug: str) -> None:
Review comment:
Yes, i agree. but this function is refactored out from dashboard function. it is tested in
https://github.com/apache/incubator-superset/blob/448a41a4e7563cafadea1e03feb5980151e8b56d/tests/security_tests.py#L665
I assume the old unit tests didn't break will be good enough.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
**Note:**
For explore_json requests, we want to cache **query results**.
For dashboard requests, we want to cache **dashboard bootstrap data**, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the **results itself are not in the dashboard bootstrap data**.
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. Since query results is not part of dashboard bootstrap data, it's not included in the dashboard cache either.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, slice entity's `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will be stored in dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
**Note:**
For explore_json requests, we want to cache query results.
For dashboard requests, we want to cache dashboard bootstrap data, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the results itself are not in the dashboard bootstrap data.
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, this means slice entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, its `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
**Note:**
For explore_json requests, we want to cache query results.
For dashboard requests, we want to cache dashboard bootstrap data, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the results itself are not in the dashboard bootstrap data.
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. Since query results is not part of dashboard bootstrap data, it's not included in the dashboard cache either.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, slice entity's `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will be stored in dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, this means slice entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. For dashboard requests, it only need slice parameters but not query results. Front-end js will use slice parameters to build query, plus dashboard filters.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, its `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
**Note:**
For explore_json requests, we want to cache query results.
For dashboard requests, we want to cache dashboard bootstrap data, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the results itself are not in the dashboard bootstrap data.
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. Since query results is not part of dashboard bootstrap data, it's not included in the dashboard cache either.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, slice entity's `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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] bkyryliuk commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496159461
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
+ """
+ Get latest changed datetime for a dashboard. The change could be dashboard
Review comment:
can we get more specific with _self type ?
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +94,26 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if get_latest_changed_on:
+ content_changed_time = get_latest_changed_on(*args, **kwargs)
+ if (
+ response
+ and response.last_modified
+ and response.last_modified.timestamp()
+ < content_changed_time.timestamp()
+ ):
+ response = None
+ else:
+ content_changed_time = datetime.utcnow()
Review comment:
add a comment here why content_changed_time is set to now()
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
**Note:**
For explore_json requests, we want to cache query results.
For dashboard requests, we want to cache dashboard bootstrap data, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the results itself are not in the dashboard bootstrap data.
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. Since query results is not part of dashboard bootstrap data, it's not included in the dashboard cache either.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, its `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 edited a comment on pull request #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=h1) Report
> Merging [#10963](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/dc893fe1b3b86e2921bb77e08a531db46b3f8e5d?el=desc) will **decrease** coverage by `0.70%`.
> The diff coverage is `71.42%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10963 +/- ##
==========================================
- Coverage 61.46% 60.76% -0.71%
==========================================
Files 382 382
Lines 24139 24154 +15
==========================================
- Hits 14836 14676 -160
- Misses 9303 9478 +175
```
| Flag | Coverage Δ | |
|---|---|---|
| #python | `60.76% <71.42%> (-0.71%)` | :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/10963?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/utils/decorators.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `53.52% <56.25%> (-1.48%)` | :arrow_down: |
| [superset/views/utils.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvdXRpbHMucHk=) | `83.07% <76.47%> (-1.24%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.46% <83.33%> (-0.01%)` | :arrow_down: |
| [superset/db\_engines/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lcy9oaXZlLnB5) | `0.00% <0.00%> (-85.72%)` | :arrow_down: |
| [superset/db\_engine\_specs/hive.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL2hpdmUucHk=) | `53.90% <0.00%> (-30.08%)` | :arrow_down: |
| [superset/db\_engine\_specs/presto.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3ByZXN0by5weQ==) | `70.85% <0.00%> (-11.44%)` | :arrow_down: |
| [superset/db\_engine\_specs/sqlite.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZGJfZW5naW5lX3NwZWNzL3NxbGl0ZS5weQ==) | `65.62% <0.00%> (-9.38%)` | :arrow_down: |
| [superset/utils/celery.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvY2VsZXJ5LnB5) | `82.14% <0.00%> (-3.58%)` | :arrow_down: |
| [superset/examples/world\_bank.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvd29ybGRfYmFuay5weQ==) | `97.10% <0.00%> (-2.90%)` | :arrow_down: |
| [superset/examples/birth\_names.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvZXhhbXBsZXMvYmlydGhfbmFtZXMucHk=) | `97.36% <0.00%> (-2.64%)` | :arrow_down: |
| ... and [9 more](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?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/10963?src=pr&el=footer). Last update [dc893fe...bcf90b9](https://codecov.io/gh/apache/incubator-superset/pull/10963?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] graceguo-supercat edited a comment on pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
**Note:**
For explore_json requests, we want to cache query results.
For dashboard requests, we want to cache dashboard bootstrap data, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the results itself are not in the dashboard bootstrap data.
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, its `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495289551
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
Review comment:
this is a little confusing: Dashboard class has a get `datasources` function. So i use it in `check_dashboard_perms` function. But this function is to group slices by datasources, and the result will be used by another feature:
https://github.com/apache/incubator-superset/blob/ba009b7c09d49f2932fd10269882c901bc020c1d/superset/views/core.py#L1626
Instead of datasource.data, datasource.data_for_slices(slices) can reduce the initial dashboard data load size.
So right now i removed this helper function from utils, and build dict in the dashboard function. But i rename datasource to `slices_by_datasources` for clarification.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495282712
##########
File path: superset/utils/decorators.py
##########
@@ -72,6 +80,12 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
if request.method == "POST":
return f(*args, **kwargs)
+ # if it is dashboard request but feature is not eabled,
+ # do not use cache
+ is_dashboard = is_dashboard_request(kwargs)
Review comment:
thanks! after introduce this `skip`, `dashboard_id` is not needed in decorator function any more.
----------------------------------------------------------------
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 edited a comment on pull request #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, this means slice entity are not modified, like query parameters, datasource used, it doesn't mean the query results is not modified. For dashboard requests, it only need slice parameters but not query results. Front-end js will use slice parameters to build query, plus dashboard filters.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, its `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change changed_on field.
----------------------------------------------------------------
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 #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=h1) Report
> Merging [#10963](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/557a303cc5a05eeeef9cfce7c98da859904cf1c2?el=desc) will **decrease** coverage by `4.04%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10963 +/- ##
==========================================
- Coverage 65.80% 61.75% -4.05%
==========================================
Files 815 433 -382
Lines 38327 14194 -24133
Branches 3600 3600
==========================================
- Hits 25221 8766 -16455
+ Misses 13002 5242 -7760
- Partials 104 186 +82
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `61.75% <ø> (+<0.01%)` | :arrow_up: |
| #python | `?` | |
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/10963?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10963/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/10963/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/10963/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/10963/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/10963/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/10963/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/10963/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/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRm9ybWF0dGVycy5qcw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvaW5kZXguanM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/setup/setupPluginsExtra.js](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwUGx1Z2luc0V4dHJhLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [542 more](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?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/10963?src=pr&el=footer). Last update [557a303...bd76db1](https://codecov.io/gh/apache/incubator-superset/pull/10963?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] bkyryliuk commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496155958
##########
File path: superset/views/utils.py
##########
@@ -490,6 +538,26 @@ def check_slice_perms(_self: Any, slice_id: int) -> None:
viz_obj.raise_for_access()
+def check_dashboard_perms(_self: Any, dashboard_id_or_slug: str) -> None:
Review comment:
a good practice is to incrementally improve the state of the code, however it will be your call here
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, this means slice entity are not modified, like query parameters, datasource used, it doesn't mean the query results is not modified. For dashboard requests, it only need slice parameters but not query results. Front-end js will use slice parameters to build query, plus dashboard filters.
#3:
annotations: dashboard has no annotation,
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change changed_on field.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, this means slice entity are not modified, like query parameters, datasource used, it doesn't mean the query results is not modified. For dashboard requests, it only need slice parameters but not query results. Front-end js will use slice parameters to build query, plus dashboard filters.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, its `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496464875
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +94,26 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if get_latest_changed_on:
+ content_changed_time = get_latest_changed_on(*args, **kwargs)
+ if (
+ response
+ and response.last_modified
+ and response.last_modified.timestamp()
+ < content_changed_time.timestamp()
+ ):
+ response = None
+ else:
+ content_changed_time = datetime.utcnow()
Review comment:
at first i thought is_stale is a good idea. but when start refactor it, i found last_modified time is needed by decorator function(to set header). So is_stale (only boolean value) is not enough.
So instead of using is_stale return true or false, I prefer to keep get_last_modified, and use last_modified time to invalid cache.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496466867
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
+ """
+ Get latest changed datetime for a dashboard. The change could be dashboard
Review comment:
I don't know what type to use here. do you have suggestion? (Sorry i am not an expert in Python)
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698043672
`etag_cache` decorator works this way:
- explore_json flow:
1. http request come in
1. check_slice_perms. No matter has cached response, if no permission, response with error.
1. if method is POST, run query (not use cache). Otherwise check if this request has cache.
1. if no cached response, run query and create a cache key for the response
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is now (response time).
- dashboard flow:
1. http request come in
1. check_dashboard_perms. No matter has cached response, if no permission, response with error.
1. if method is POST, or feature is not enabled, run dashboard function to build response (not use cache). Otherwise check if this request has cache.
1. if no cached response, run dashboard function.
1. if has cache, compare cached time with dashboard last modified time: it could be dashboard's metadata was changed (dashboard's changed_on), or any of its slices was changed (slice's changed_on). If cache is stale, run dashboard function.
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard' changed_on and its slices changed_on)
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698043672
`etag_cache` decorator works this way:
- explore_json flow:
1. http request come in
1. check_slice_perms
1. if method is POST, run query (not use cache). Otherwise check if this request has cache.
1. if no cached response, run query and create a cache key for the response
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is now (response time).
- dashboard flow:
1. http request come in
1. check_dashboard_perms
1. if method is POST, or feature is not enabled, run dashboard function to build response (not use cache). Otherwise check if this request has cache.
1. if no cached response, run dashboard function.
1. if has cache, compare cached time with dashboard last modified time: it could be dashboard's changed_on > last cache_time or any slice's changed_on > last_cache_time. If cache is stale, run dashboard function.
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard and its slices)
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495283325
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +103,25 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if check_latest_changed_on:
+ latest_changed_on = check_latest_changed_on(*args, **kwargs)
+ if response and response.last_modified:
+ latest_record = response.last_modified.replace(
+ tzinfo=timezone.utc
+ ).astimezone(tz=None)
+ if latest_changed_on.timestamp() > latest_record.timestamp():
+ response = None
Review comment:
fixed.
----------------------------------------------------------------
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] bkyryliuk commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r494451542
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
Review comment:
looks like a good candidate for the Dashboard class method
##########
File path: superset/views/utils.py
##########
@@ -298,6 +306,46 @@ def get_time_range_endpoints(
CONTAINER_TYPES = ["COLUMN", "GRID", "TABS", "TAB", "ROW"]
+def get_dashboard(dashboard_id_or_slug: str,) -> Dashboard:
+ session = db.session()
+ qry = session.query(Dashboard)
+ if dashboard_id_or_slug.isdigit():
+ qry = qry.filter_by(id=int(dashboard_id_or_slug))
+ else:
+ qry = qry.filter_by(slug=dashboard_id_or_slug)
+ dashboard = qry.one_or_none()
+
+ if not dashboard:
+ abort(404)
+
+ return dashboard
+
+
+def get_datasources_from_dashboard(
+ dashboard: Dashboard,
+) -> DefaultDict[Any, List[Any]]:
+ datasources = defaultdict(list)
+ for slc in dashboard.slices:
+ datasource = slc.datasource
+ if datasource:
+ datasources[datasource].append(slc)
+ return datasources
+
+
+def get_dashboard_latest_changed_on(_self: Any, dashboard_id_or_slug: str) -> datetime:
Review comment:
what is _self here? ideally we should avoid Any types
##########
File path: superset/utils/decorators.py
##########
@@ -34,6 +34,10 @@
logger = logging.getLogger(__name__)
+def is_dashboard_request(kwargs: Any) -> bool:
+ return kwargs.get("dashboard_id_or_slug") is not None
Review comment:
doesn't seem robust, it it possible to validate via uri path or just pass a param ?
##########
File path: superset/views/utils.py
##########
@@ -490,6 +538,26 @@ def check_slice_perms(_self: Any, slice_id: int) -> None:
viz_obj.raise_for_access()
+def check_dashboard_perms(_self: Any, dashboard_id_or_slug: str) -> None:
Review comment:
best practice is to have a unit test for every function, it would be great if you could add some
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +103,25 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if check_latest_changed_on:
+ latest_changed_on = check_latest_changed_on(*args, **kwargs)
+ if response and response.last_modified:
+ latest_record = response.last_modified.replace(
+ tzinfo=timezone.utc
Review comment:
this assumes that superset server runs in utc zone, it may be safer to make it as a superset config variable
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698061914
**Note:**
For explore_json requests, we want to cache query results.
For dashboard requests, we want to cache dashboard bootstrap data, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the results itself are not in the dashboard bootstrap data.
This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
#2
if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. Since query results is not included in the dashboard bootstrap data, it's not in the dashboard cache.
#3:
annotations: dashboard has no annotation. If slice's annotation was changed, its `params` and `changed_on` attribute will be changed.
changed to the default filters, css rules, etc: these change will affect dashboard metadata, and will change dashboard `changed_on` attribute.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495284771
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +103,25 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if check_latest_changed_on:
+ latest_changed_on = check_latest_changed_on(*args, **kwargs)
+ if response and response.last_modified:
+ latest_record = response.last_modified.replace(
+ tzinfo=timezone.utc
Review comment:
this convert, .replace(tzinfo) is not necessary, removed.
----------------------------------------------------------------
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] bkyryliuk commented on pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
bkyryliuk commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698451673
> **Note:**
> For explore_json requests, we want to cache **query results**.
>
> For dashboard requests, we want to cache **dashboard bootstrap data**, which includes datasource metadata, slices parameters, etc. Dashboard front-end js use datasource metadata, slice parameters, and dashboard filters to build query and fetch query results, the **results itself are not in the dashboard bootstrap data**.
>
> This PR will only focus on dashboard's cache stale logic. i do not want to change current slice cache behavior.
>
> #2
> if chart **was not modified for ~10 hours**, I assume this means `slice` entity are not modified, like query parameters, datasource used, it doesn't mean the **query results** is not modified. Since query results is not part of dashboard bootstrap data, it's not included in the dashboard cache either.
>
> #3:
> annotations: dashboard has no annotation. If slice's annotation was changed, slice entity's `params` and `changed_on` attribute will be changed.
> changed to the default filters, css rules, etc: these change will be stored in dashboard metadata, and will change dashboard `changed_on` attribute.
Big thanks for the explanation!
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496295822
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +94,26 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if get_latest_changed_on:
+ content_changed_time = get_latest_changed_on(*args, **kwargs)
+ if (
+ response
+ and response.last_modified
+ and response.last_modified.timestamp()
+ < content_changed_time.timestamp()
+ ):
+ response = None
+ else:
+ content_changed_time = datetime.utcnow()
Review comment:
We use this `content_changed_time` as cache's last_modified time.
for dashboard `content_changed_time` is dashboard entity's latest updated time (like metadata, chart metadata changed time etc). this data is from a callback function.
for explore_json, the cache is query results and there is no entity's latest modified time to use. so we use request time (now) as cache's 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] ktmud commented on a change in pull request #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r493791375
##########
File path: superset/utils/decorators.py
##########
@@ -72,6 +80,12 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
if request.method == "POST":
return f(*args, **kwargs)
+ # if it is dashboard request but feature is not eabled,
+ # do not use cache
+ is_dashboard = is_dashboard_request(kwargs)
Review comment:
Instead of adding a helper function, maybe we can just expand `dashboard_id_or_slug` in `wrapper` and make this code more transparent?
```python
def wrapper(*args: Any, dashboard_id_or_slug: str=None, **kwargs: Any) -> ETagResponseMixin:
# ...
is_dashboard = dashboard_id_or_slug is not None
```
Better yet, we should probably aim for keeping all dashboard specific logics out of `etag_cache` so it stays generic. Maybe add a `skip=` parameter that runs the feature flag check.
```python
@etag_cache(skip=lambda: is_feature_enabled("ENABLE_DASHBOARD_ETAG_HEADER"))
```
```python
def etag_cache(
max_age: int,
check_perms: Callable[..., Any],
skip: Optional[Callable[..., Any]] = None,
) -> Callable[..., Any]:
def decorator(f: Callable[..., Any]) -> Callable[..., Any]:
def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
check_perms(*args, **kwargs)
if request.method == "POST" or (skip and skip(*args, **kwargs)):
return f(*args, **kwargs)
```
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +103,25 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if check_latest_changed_on:
+ latest_changed_on = check_latest_changed_on(*args, **kwargs)
+ if response and response.last_modified:
+ latest_record = response.last_modified.replace(
+ tzinfo=timezone.utc
+ ).astimezone(tz=None)
+ if latest_changed_on.timestamp() > latest_record.timestamp():
+ response = None
Review comment:
Can probably rename `check_latest_changed_on ` to `get_latest_changed_on` and do
```python
if get_latest_changed_on:
latest_changed_on = get_latest_changed_on(*args, **kwargs)
if response and response.last_modified and response.last_modified < latest_changed_on:
response = None
else:
latest_changed_on = datetime.utcnow()
```
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496461532
##########
File path: superset/utils/decorators.py
##########
@@ -46,7 +46,12 @@ def stats_timing(stats_key: str, stats_logger: BaseStatsLogger) -> Iterator[floa
stats_logger.timing(stats_key, now_as_float() - start_ts)
-def etag_cache(max_age: int, check_perms: Callable[..., Any]) -> Callable[..., Any]:
+def etag_cache(
+ max_age: int,
+ check_perms: Callable[..., Any],
+ get_latest_changed_on: Optional[Callable[..., Any]] = None,
Review comment:
agree. rename it to `get_last_modified`.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698043672
`etag_cache` decorator works this way:
- explore_json flow:
1. http request come in
1. check_slice_perms
1. if method is POST, run query (not use cache). Otherwise check if this request has cache.
1. if no cached response, run query and create a cache key for the response
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is now (response time).
- dashboard flow:
1. http request come in
1. check_dashboard_perms
1. if method is POST, or feature is not enabled, run dashboard function to build response (not use cache). Otherwise check if this request has cache.
1. if no cached response, run dashboard function.
1. if has cache, compare cached time with dashboard last modified time: it could be dashboard's metadata was changed (dashboard's changed_on), or any of its slices was changed (slice's changed_on). If cache is stale, run dashboard function.
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard and its slices)
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496313536
##########
File path: superset/utils/decorators.py
##########
@@ -46,7 +46,12 @@ def stats_timing(stats_key: str, stats_logger: BaseStatsLogger) -> Iterator[floa
stats_logger.timing(stats_key, now_as_float() - start_ts)
-def etag_cache(max_age: int, check_perms: Callable[..., Any]) -> Callable[..., Any]:
+def etag_cache(
+ max_age: int,
+ check_perms: Callable[..., Any],
+ get_latest_changed_on: Optional[Callable[..., Any]] = None,
Review comment:
Can we rename `get_latest_changed_on` to `get_last_modified` just to be more consistent with the response attribute? Imagine in future refactor `response.last_modified` is renamed to something else, you would know this function is definitely related by searching for `last_modified`.
##########
File path: superset/views/core.py
##########
@@ -1585,49 +1598,32 @@ def publish( # pylint: disable=no-self-use
return json_success(json.dumps({"published": dash.published}))
@has_access
+ @etag_cache(
+ 0,
+ check_perms=check_dashboard_perms,
+ get_latest_changed_on=get_dashboard_changedon_dt,
Review comment:
Nit: it's a little weird to have both `changed_on` and `changedon`.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698043672
`etag_cache` decorator works this way:
- explore_json flow:
1. http request come in
1. check_slice_perms
1. if method is POST, run query (not use cache). Otherwise check if this request has cache.
1. if no cached response, run query and create a cache key for the response
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is now (response time).
- dashboard flow:
1. http request come in
1. check_dashboard_perms
1. if method is POST, or feature is not enabled, run dashboard function to build response (not use cache). Otherwise check if this request has cache.
1. if no cached response, run dashboard function.
1. if has cache, compare cached time with dashboard last modified time: it could be dashboard's metadata was changed (dashboard's changed_on), or any of its slices was changed (slice's changed_on). If cache is stale, run dashboard function.
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard' changed_on and its slices changed_on)
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
ktmud commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r496315904
##########
File path: superset/views/core.py
##########
@@ -1585,49 +1598,32 @@ def publish( # pylint: disable=no-self-use
return json_success(json.dumps({"published": dash.published}))
@has_access
+ @etag_cache(
+ 0,
+ check_perms=check_dashboard_perms,
+ get_latest_changed_on=get_dashboard_changedon_dt,
Review comment:
Nit: it's a little weird to have both `changed_on` and `changedon`, but up to your.
----------------------------------------------------------------
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 #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=h1) Report
> Merging [#10963](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/557a303cc5a05eeeef9cfce7c98da859904cf1c2?el=desc) will **decrease** coverage by `4.04%`.
> The diff coverage is `n/a`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10963 +/- ##
==========================================
- Coverage 65.80% 61.75% -4.05%
==========================================
Files 815 433 -382
Lines 38327 14194 -24133
Branches 3600 3600
==========================================
- Hits 25221 8766 -16455
+ Misses 13002 5242 -7760
- Partials 104 186 +82
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `61.75% <ø> (+<0.01%)` | :arrow_up: |
| #python | `?` | |
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/10963?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10963/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/10963/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/10963/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/10963/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/10963/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/10963/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/10963/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/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwRm9ybWF0dGVycy5qcw==) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/explore/reducers/index.js](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL2V4cGxvcmUvcmVkdWNlcnMvaW5kZXguanM=) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| [superset-frontend/src/setup/setupPluginsExtra.js](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwUGx1Z2luc0V4dHJhLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [542 more](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?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/10963?src=pr&el=footer). Last update [557a303...bd76db1](https://codecov.io/gh/apache/incubator-superset/pull/10963?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] codecov-commenter edited a comment on pull request #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495284455
##########
File path: superset/utils/decorators.py
##########
@@ -34,6 +34,10 @@
logger = logging.getLogger(__name__)
+def is_dashboard_request(kwargs: Any) -> bool:
+ return kwargs.get("dashboard_id_or_slug") is not None
Review comment:
after introduce this skip, `dashboard_id_or_slug` is not needed in decorator function any more. this check is removed.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat commented on a change in pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#discussion_r495284771
##########
File path: superset/utils/decorators.py
##########
@@ -89,13 +103,25 @@ def wrapper(*args: Any, **kwargs: Any) -> ETagResponseMixin:
raise
logger.exception("Exception possibly due to cache backend.")
+ # if cache is stale?
+ if check_latest_changed_on:
+ latest_changed_on = check_latest_changed_on(*args, **kwargs)
+ if response and response.last_modified:
+ latest_record = response.last_modified.replace(
+ tzinfo=timezone.utc
Review comment:
this convert, `.replace(tzinfo)` is not necessary, removed.
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698043672
`etag_cache` decorator works this way:
- explore_json flow:
1. http request come in
1. check_slice_perms. Even if has cache, if no permission, response with error.
1. if method is POST, run query (not use cache). Otherwise check if this request has cache.
1. if no cached response, run query and create a cache key for the response
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is now (response time).
- dashboard flow:
1. http request come in
1. check_dashboard_perms. Even if has cache, if no permission, response with error.
1. if method is POST, or feature is not enabled, run dashboard function to build response (not use cache). Otherwise check if this request has cache.
1. if no cached response, run dashboard function.
1. if has cache, compare cached time with dashboard last modified time: it could be dashboard's metadata was changed (dashboard's changed_on), or any of its slices was changed (slice's changed_on). If cache is stale, run dashboard function.
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard' changed_on and its slices changed_on)
----------------------------------------------------------------
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 #10963: feat: enable ETag header for dashboard GET requests
Posted by GitBox <gi...@apache.org>.
graceguo-supercat edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-698043672
`etag_cache` decorator works this way:
- explore_json flow:
1. http request come in
1. check_slice_perms
1. if method is POST, run query (not use cache). Otherwise check if this request has cache.
1. if no cached response, run query and create a cache key for the response
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is now (response time).
- dashboard flow:
1. http request come in
1. check_dashboard_perms. even if has cache, if no permission, response with error.
1. if method is POST, or feature is not enabled, run dashboard function to build response (not use cache). Otherwise check if this request has cache.
1. if no cached response, run dashboard function.
1. if has cache, compare cached time with dashboard last modified time: it could be dashboard's metadata was changed (dashboard's changed_on), or any of its slices was changed (slice's changed_on). If cache is stale, run dashboard function.
1. send response to client-side, with eTag header, last_modified header and expiration time header. last_modified time is max of (dashboard' changed_on and its slices changed_on)
----------------------------------------------------------------
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 edited a comment on pull request #10963: [WIP]feat: enable ETag header for dashboard page load
Posted by GitBox <gi...@apache.org>.
codecov-commenter edited a comment on pull request #10963:
URL: https://github.com/apache/incubator-superset/pull/10963#issuecomment-695946006
# [Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=h1) Report
> Merging [#10963](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=desc) into [master](https://codecov.io/gh/apache/incubator-superset/commit/557a303cc5a05eeeef9cfce7c98da859904cf1c2?el=desc) will **decrease** coverage by `4.23%`.
> The diff coverage is `70.17%`.
[](https://codecov.io/gh/apache/incubator-superset/pull/10963?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #10963 +/- ##
==========================================
- Coverage 65.80% 61.56% -4.24%
==========================================
Files 815 815
Lines 38327 38358 +31
Branches 3600 3600
==========================================
- Hits 25221 23617 -1604
- Misses 13002 14555 +1553
- Partials 104 186 +82
```
| Flag | Coverage Δ | |
|---|---|---|
| #cypress | `?` | |
| #javascript | `61.75% <ø> (+<0.01%)` | :arrow_up: |
| #python | `61.45% <70.17%> (+<0.01%)` | :arrow_up: |
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/10963?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [superset/config.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvY29uZmlnLnB5) | `90.42% <0.00%> (ø)` | |
| [superset/utils/decorators.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdXRpbHMvZGVjb3JhdG9ycy5weQ==) | `53.52% <56.25%> (-1.48%)` | :arrow_down: |
| [superset/views/utils.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvdXRpbHMucHk=) | `83.07% <76.47%> (-1.24%)` | :arrow_down: |
| [superset/views/core.py](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQvdmlld3MvY29yZS5weQ==) | `74.46% <83.33%> (-0.01%)` | :arrow_down: |
| [superset-frontend/src/SqlLab/App.jsx](https://codecov.io/gh/apache/incubator-superset/pull/10963/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/10963/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/10963/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/10963/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/10963/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/10963/diff?src=pr&el=tree#diff-c3VwZXJzZXQtZnJvbnRlbmQvc3JjL3NldHVwL3NldHVwQ29sb3JzLmpz) | `0.00% <0.00%> (-100.00%)` | :arrow_down: |
| ... and [171 more](https://codecov.io/gh/apache/incubator-superset/pull/10963/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-superset/pull/10963?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/10963?src=pr&el=footer). Last update [557a303...d1f2e55](https://codecov.io/gh/apache/incubator-superset/pull/10963?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