-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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
feat: Reuse Dashboard redux data in Explore #20850
Conversation
@jinghua-qa Could you please help with testing? This PR was previously reverted due to a bug that went unnoticed, I'm hoping to avoid repeating this mistake. |
is_managed_externally = fields.Boolean( | ||
description=is_managed_externally_description | ||
) | ||
owners = fields.List(fields.Nested(UserSchema)) |
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.
a minor suggestion from recent fixing. the owners
fields suggest that align with Dashboard field to owners = fields.List(fields.Dict())
.
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.
Codecov Report
@@ Coverage Diff @@
## master #20850 +/- ##
==========================================
- Coverage 66.17% 66.14% -0.03%
==========================================
Files 1757 1757
Lines 66867 66866 -1
Branches 7077 7077
==========================================
- Hits 44246 44226 -20
- Misses 20812 20831 +19
Partials 1809 1809
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. |
I decided to close this PR - the performance gain from reusing the dashboard redux data was negligible at the cost of increased code complexity, which introduces a greater risk of bugs and regressions. |
SUMMARY
Reopen #20668
Currently when user open Explore, we always send a request to
/v1/explore
endpoint, which assembles Explore initial data - slice, form_data and datasource.When user goes to Explore from Dashboard (by clicking "Edit chart" or chart title), most of the data needed to assemble Explore should already be available in Redux store.
This PR implements using data from Dashboard Redux store to initialize Explore without calling
/v1/explore
endpoint.If user opens Explore from Dashboard, check if slice, formData, datasourceId and datasourceType are available. If they are, fetch datasource metadata and hydrate Explore using slice, formData and datasource.
If they are not available, call
/v1/explore
and hydrate Explore.We can't reuse datasource from Dashboard. Datasources metadata on Dashboard is trimmed so that datasource contains only columns and metrics used by charts on that dashboard. In Explore we need to be able to access all columns and metrics, which means we must fetch those from API.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
/v1/explore/
endpoint/v1/explore
ADDITIONAL INFORMATION