fix(cache): Add cache warmup for non-legacy charts #24671

john-bodley · 2023-07-12T05:42:20Z

SUMMARY

This PR is a rehash of #23012 which was later reverted by #23579 in combination with #23569.

The logic defined here is the inverse of #23012, i.e., rather than trying to identify if a chart is a non-legacy chart (via the presence of the query-context—which was problematic), instead we check if the chart is a supported legacy chart/viz type.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

Added unit tests.

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

cc: @alexryndin @dheeraj-jaiswal-lowes

codecov · 2023-07-12T05:51:21Z

Codecov Report

Merging #24671 (de35ed9) into master (aa01b51) will increase coverage by 0.00%.
The diff coverage is 96.66%.

❗ Current head de35ed9 differs from pull request most recent head 74f25f0. Consider uploading reports for the commit 74f25f0 to get more accurate results

@@           Coverage Diff           @@
##           master   #24671   +/-   ##
=======================================
  Coverage   68.89%   68.89%           
=======================================
  Files        1901     1901           
  Lines       73927    73925    -2     
  Branches     8183     8183           
=======================================
- Hits        50932    50931    -1     
+ Misses      20874    20873    -1     
  Partials     2121     2121

Flag	Coverage Δ
hive	`54.16% <23.33%> (+0.01%)`	⬆️
mysql	`79.21% <96.66%> (+<0.01%)`	⬆️
postgres	`79.30% <96.66%> (+<0.01%)`	⬆️
presto	`54.06% <23.33%> (+0.01%)`	⬆️
python	`83.32% <96.66%> (+<0.01%)`	⬆️
sqlite	`77.88% <96.66%> (+<0.01%)`	⬆️
unit	`54.88% <23.33%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/charts/commands/warm_up_cache.py	`98.14% <96.00%> (+0.64%)`	⬆️
superset/charts/commands/export.py	`94.11% <100.00%> (ø)`
superset/views/core.py	`69.60% <100.00%> (-0.75%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

john-bodley · 2023-07-12T05:44:32Z

superset/charts/commands/warm_up_cache.py

        try:
            form_data = get_form_data(chart.id, use_slice_data=True)[0]
-            if self._dashboard_id:


This logic (for the most part) has just been indented.

john-bodley · 2023-07-12T05:45:23Z

superset/views/core.py

@@ -825,43 +827,15 @@ def warm_up_cache(  # pylint: disable=too-many-locals,no-self-use
                .all()
            )

-        result = []


Adhering to the DRY principle.

john-bodley · 2023-07-12T05:45:56Z

tests/integration_tests/charts/api_tests.py

@@ -14,37 +14,41 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-# isort:skip_file


I have no idea why this was here.

john-bodley · 2023-07-12T05:46:27Z

tests/integration_tests/charts/api_tests.py

@@ -1710,12 +1714,16 @@ def test_gets_owned_created_favorited_by_me_filter(self):
        assert data["result"][0]["slice_name"] == "name0"
        assert data["result"][0]["datasource_id"] == 1

-    @pytest.mark.usefixtures(
-        "load_energy_table_with_slice", "load_birth_names_dashboard_with_slices"
+    @parameterized.expand(


Ensure we can successfully warm up legacy and non-legacy charts.

john-bodley · 2023-07-12T05:49:09Z

tests/integration_tests/core_tests.py

@@ -14,72 +14,67 @@
 # KIND, either express or implied.  See the License for the
 # specific language governing permissions and limitations
 # under the License.
-# isort:skip_file


I have no idea why this was here.

john-bodley · 2023-07-12T05:49:55Z

tests/integration_tests/charts/api_tests.py

@@ -1710,12 +1714,16 @@ def test_gets_owned_created_favorited_by_me_filter(self):
        assert data["result"][0]["slice_name"] == "name0"
        assert data["result"][0]["datasource_id"] == 1

-    @pytest.mark.usefixtures(
-        "load_energy_table_with_slice", "load_birth_names_dashboard_with_slices"


The load_energy_table_with_slice fixture is not required.

john-bodley · 2023-07-12T06:18:09Z

superset/charts/commands/warm_up_cache.py

+            if form_data.get("viz_type") in viz_types:
+                # Legacy visualizations.
+                if not chart.datasource:
+                    raise ChartInvalidError("Chart's datasource does not exist")


Raising a ChartInvalidError seems more appropriate than raising an Exception.

john-bodley · 2023-07-12T06:28:50Z

superset/views/core.py

@@ -769,7 +770,8 @@ def save_or_overwrite_slice(
    @api
    @has_access_api
    @expose("/warm_up_cache/", methods=("GET",))
-    def warm_up_cache(  # pylint: disable=too-many-locals,no-self-use
+    @deprecated(new_target="api/v1/chart/warm_up_cache/")


@michael-s-molina I gather it's now too late to remove this endpoint in 3.0 given there's a new target.

it would be great ;)

Let's remove it in 4.0 given that the breaking window is closed.

john-bodley · 2023-07-12T06:29:59Z

tests/integration_tests/charts/commands_tests.py

@@ -96,52 +96,7 @@ def test_export_chart_command(self, mock_g):
            "dataset_uuid": str(example_chart.table.uuid),
            "uuid": str(example_chart.uuid),
            "version": "1.0.0",
-        }


Adhering to the logic in #23012.

michael-s-molina

Thank you for the PR @john-bodley. I left some first-pass comments.

michael-s-molina · 2023-07-12T11:51:42Z

superset/charts/commands/export.py

@@ -34,7 +34,7 @@


 # keys present in the standard export that are not needed
-REMOVE_KEYS = ["datasource_type", "datasource_name", "query_context", "url_params"]
+REMOVE_KEYS = ["datasource_type", "datasource_name", "url_params"]


Why did you add the query_context to the payload of exported charts?

@michael-s-molina this logic was extracted from #23012.

michael-s-molina · 2023-07-12T11:56:45Z

superset/charts/commands/warm_up_cache.py

-            status = payload["status"]
+
+            if form_data.get("viz_type") in viz_types:
+                # Legacy visualizations.


The problem with this approach is that we have non-legacy charts that are also present in viz_types under the same key like table and big_number.

@michael-s-molina that may explain why testing with the table viz type was problematic.

Should the table, big_number, etc. viz types be removed from superset.viz? Alternatively if they’re still required, i.e., we simultaneously support both legacy and non-legacy versions, how does Superset differentiate between the two? Is it the presence of the query context?

Should the table, big_number, etc. viz types be removed from superset.viz?

Yes. Let's remove them.

See #24694 which is now a precursor to this PR.

john-bodley · 2023-07-18T23:49:25Z

@michael-s-molina this PR is now ready for re-review.

michael-s-molina · 2023-07-19T16:19:39Z

superset/charts/commands/warm_up_cache.py

+                    status = query["status"]
+
+                    if error is not None:
+                        break
        except Exception as ex:  # pylint: disable=broad-except
            error = error_msg_from_exception(ex)
            status = None


Do we have a status for errors?

It seems not. This is the same logic as previously.

michael-s-molina

LGTM. I left a non-blocking comment about the status when there's an error.

stepcheunghk · 2023-07-20T04:06:11Z

@john-bodley Thank you so much. This one is very helpful!

(cherry picked from commit 5f49e0f)

stepcheunghk · 2023-07-27T06:33:40Z

Hi @john-bodley, we have found something funny. We noticed that some charts have been cached by cache warmup job, which is good. But if we click the dashboard, it seems those charts in the dashboard are not using the warmup cache. Do you have any idea? Thank you.

dheeraj281 · 2023-07-27T11:03:49Z

Hi @stepcheunghk , Are there any native filters or other filters applied to the chart as part of dashboard?

stepcheunghk · 2023-07-27T11:22:34Z

Hi @dheeraj281 , you are right. Definitely there are some filters applied to the chart/ dashboard. What should I do now? Thanks man.

dheeraj281 · 2023-07-27T12:04:50Z

@stepcheunghk Yes, thats the reason.. It refreshes the chart objects related to the dashboard. When native filter is applied on the dashboard, its a different query now. If you dont keep any filter with default value, it should work as expected.

stepcheunghk · 2023-07-27T12:07:52Z

@dheeraj281 I see. Thanks for your prompt reply and clear explanation. 🙂

oriabyi · 2023-10-02T12:34:53Z

Hi @dheeraj281 @stepcheunghk
if I have a dashboard with a native filter with a default value and also in it, I have charts with filters inside (with clauses where/having), how should I do cache_warmup so it would refresh my dashboard with default values and all charts inside it with their own filters?

dheeraj281 · 2023-10-03T05:10:53Z

Hi @oriabyi I am not aware of any such functionality right now. As discussed above It refreshes the chart objects related to the dashboard. it basically loads the chart url with force=true. One way of refreshing dashboard could be a separate selenium script that loads the dashboard url with url param "force=true"

Even we had this requirement of refreshing a dashboard with a native filter with a default value and also in it. To handle that in superset for our internal use, I have changed the cache-refresh functionality to work from the backend instead of using selenium and chart url approach. In backend it will identify charts and all the associated dashboard level filters of it and runs the exact same query which is their at dashboard level using chartDataCommand. If many users wants this, I can create a PR for the same.
cc: @rusackas @villebro

oriabyi · 2023-10-03T18:32:54Z

@dheeraj281, thank you, it helped a lot, truly appreciate it.

pull-request-size bot added the size/L label Jul 12, 2023

john-bodley mentioned this pull request Jul 12, 2023

fix(warm_up_cache): JSON serialization #23569

Closed

9 tasks

john-bodley commented Jul 12, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-cache branch 3 times, most recently from a097d67 to aa07137 Compare July 12, 2023 06:10

john-bodley requested review from dpgaspar, villebro and michael-s-molina July 12, 2023 06:11

john-bodley force-pushed the john-bodley--fix-cache branch from aa07137 to 05904e6 Compare July 12, 2023 06:17

john-bodley commented Jul 12, 2023

View reviewed changes

john-bodley marked this pull request as ready for review July 12, 2023 06:18

john-bodley commented Jul 12, 2023

View reviewed changes

john-bodley force-pushed the john-bodley--fix-cache branch from 05904e6 to abfb6c1 Compare July 12, 2023 06:53

michael-s-molina reviewed Jul 12, 2023

View reviewed changes

john-bodley added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 12, 2023

This was referenced Jul 12, 2023

chore: Remove obsolete legacy visualizations #24675

Closed

chore: Remove obsolete legacy visualizations #24694

Merged

john-bodley force-pushed the john-bodley--fix-cache branch 4 times, most recently from e90379d to aaefe70 Compare July 18, 2023 16:40

fix: cache warmup for non-legacy charts

74f25f0

john-bodley force-pushed the john-bodley--fix-cache branch from aaefe70 to 74f25f0 Compare July 18, 2023 21:30

john-bodley requested a review from michael-s-molina July 18, 2023 23:49

john-bodley changed the title ~~fix: cache warmup for non-legacy charts~~ fix(cache): Add cache warmup for non-legacy charts Jul 19, 2023

michael-s-molina reviewed Jul 19, 2023

View reviewed changes

michael-s-molina approved these changes Jul 19, 2023

View reviewed changes

john-bodley merged commit 5f49e0f into apache:master Jul 19, 2023

michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023

fix(cache): Add cache warmup for non-legacy charts (#24671)

a3138f2

(cherry picked from commit 5f49e0f)

john-bodley deleted the john-bodley--fix-cache branch November 13, 2023 19:28

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

fix(cache): Add cache warmup for non-legacy charts (apache#24671)

d9c532f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cache): Add cache warmup for non-legacy charts #24671

fix(cache): Add cache warmup for non-legacy charts #24671

john-bodley commented Jul 12, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

john-bodley Jul 12, 2023

dpgaspar Jul 13, 2023

michael-s-molina Jul 13, 2023

john-bodley Jul 12, 2023

michael-s-molina left a comment

michael-s-molina Jul 12, 2023

john-bodley Jul 12, 2023

michael-s-molina Jul 12, 2023

john-bodley Jul 12, 2023

michael-s-molina Jul 12, 2023

john-bodley Jul 14, 2023

john-bodley commented Jul 18, 2023

michael-s-molina Jul 19, 2023

john-bodley Jul 19, 2023

michael-s-molina left a comment

stepcheunghk commented Jul 20, 2023

stepcheunghk commented Jul 27, 2023

dheeraj281 commented Jul 27, 2023

stepcheunghk commented Jul 27, 2023

dheeraj281 commented Jul 27, 2023 •

edited

Loading

stepcheunghk commented Jul 27, 2023

oriabyi commented Oct 2, 2023

dheeraj281 commented Oct 3, 2023

oriabyi commented Oct 3, 2023

fix(cache): Add cache warmup for non-legacy charts #24671

fix(cache): Add cache warmup for non-legacy charts #24671

Conversation

john-bodley commented Jul 12, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Jul 12, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-bodley commented Jul 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina left a comment

Choose a reason for hiding this comment

stepcheunghk commented Jul 20, 2023

stepcheunghk commented Jul 27, 2023

dheeraj281 commented Jul 27, 2023

stepcheunghk commented Jul 27, 2023

dheeraj281 commented Jul 27, 2023 • edited Loading

stepcheunghk commented Jul 27, 2023

oriabyi commented Oct 2, 2023

dheeraj281 commented Oct 3, 2023

oriabyi commented Oct 3, 2023

john-bodley commented Jul 12, 2023 •

edited

Loading

codecov bot commented Jul 12, 2023 •

edited

Loading

dheeraj281 commented Jul 27, 2023 •

edited

Loading