-
Notifications
You must be signed in to change notification settings - Fork 14.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(revert): "fix: cache warmup solution non legacy charts. (#23012)" #23579
fix(revert): "fix: cache warmup solution non legacy charts. (#23012)" #23579
Conversation
This reverts commit e755b4f.
Codecov Report
@@ Coverage Diff @@
## master #23579 +/- ##
=======================================
Coverage 67.71% 67.72%
=======================================
Files 1916 1916
Lines 74014 74007 -7
Branches 8039 8039
=======================================
- Hits 50122 50118 -4
+ Misses 21843 21840 -3
Partials 2049 2049
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - I created a filter box, and lo and behold, it indeed has a query context, and based on my quick inspection the payload is invalid (=would return a 400 if sent to the chart data endpoint).
After this revert is merged I can work on a new PR so that
- saving a legacy chart sets
query_context
toNULL
and fixes existing chart metadata (db migration) - reintroduces the functionality from the reverted PR
Ok thanks for the heads up @john-bodley . I'll make sure to incorporate the work from all prior PRs in my fix 👍 |
…3012)" (apache#23579) (cherry picked from commit b58d17f)
SUMMARY
This reverts #23012 which per #23012 (comment) introduced two known regressions. Granted we could try to roll forward with a formulation which works for both legacy and non-legacy charts, but in the interim I think it's prudent we ensure we don't regress whilst acknowledging the fact that cache warmup for non-legacy charts is still problematic.
cc: @dheeraj-jaiswal-lowes
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION