Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: cache warmup unable to login (#9597, #18933) #20387

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ensky
Copy link
Contributor

@ensky ensky commented Jun 15, 2022

SUMMARY

Currently cache warmup feature is unusable due to #9597 & #18933
This implement intend to fix it by following changes:

  • Use security_manager.find_user to find a user credential
  • Use WebDriverProxy to perform warmups
    • In Superset, front-end tend to send multiple api with complex filter and many options, which is difficult to replay via simple request apis. In order to reduce the probability to cache the wrong slices or with wrong parameters, we use web driver to simulate a user's real behavior.
  • Refine WebDriverProxy to support multiple operations without re-initialize web driver
  • Cache toward dashboard instead of slices, because slices cache aren't always able to use in dashboards

TESTING INSTRUCTIONS

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

@ensky ensky changed the title fix: cache warmup unable to login (#9597) fix: cache warmup unable to login (#9597, #18933) Jun 15, 2022
@ensky ensky force-pushed the master branch 9 times, most recently from c17d9c1 to 200e3d1 Compare June 15, 2022 06:34
@codecov
Copy link

codecov bot commented Jun 16, 2022

Codecov Report

Attention: Patch coverage is 78.57143% with 9 lines in your changes missing coverage. Please review.

Project coverage is 66.69%. Comparing base (ccba5b2) to head (42b11f6).
Report is 4105 commits behind head on master.

Files with missing lines Patch % Lines
superset/tasks/cache.py 54.54% 5 Missing ⚠️
superset/utils/screenshots.py 25.00% 3 Missing ⚠️
superset/utils/webdriver.py 96.15% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #20387      +/-   ##
==========================================
- Coverage   66.70%   66.69%   -0.02%     
==========================================
  Files        1739     1739              
  Lines       65137    65121      -16     
  Branches     6897     6897              
==========================================
- Hits        43452    43433      -19     
- Misses      19932    19935       +3     
  Partials     1753     1753              
Flag Coverage Δ
hive 53.72% <30.95%> (+0.02%) ⬆️
mysql 82.27% <78.57%> (-0.02%) ⬇️
postgres 82.34% <78.57%> (-0.02%) ⬇️
presto 53.58% <30.95%> (+0.02%) ⬆️
python 82.76% <78.57%> (-0.02%) ⬇️
sqlite 82.07% <78.57%> (-0.02%) ⬇️
unit 50.19% <23.80%> (+0.03%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ensky
Copy link
Contributor Author

ensky commented Aug 4, 2022

I'm going to mention maintainers randomly due to pending reviews. Any feedback is welcome.
@kgabryje @diegomedina248

@ensky
Copy link
Contributor Author

ensky commented Oct 25, 2022

Closed because of #21076

@ensky ensky closed this Oct 25, 2022
@ensky
Copy link
Contributor Author

ensky commented Oct 25, 2022

#21076 (comment)
re-open because of this comment. Oops. :P

@ensky ensky reopened this Oct 25, 2022
@musicmuthu
Copy link

Some one please when this fix will be released?

@rusackas
Copy link
Member

rusackas commented Feb 6, 2024

Wow, sorry this slid under so many people's radar for so long. This PR is in need of a rebase if you're still willing to do that, but I'll add some reviewers for good measure in case anyone is able/willing to assess the situation and carry any necessary work forward.

@rusackas
Copy link
Member

rusackas commented Sep 9, 2024

This PR is still badly in need of a rebase, but I still hope we can get it across the finish line. I'll move it to Draft mode for now, but @ensky please mark it as "ready for review" if/when it is ready.

@rusackas rusackas marked this pull request as draft September 9, 2024 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants