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

Reduce dashboard position_json data size #5543

Conversation

graceguo-supercat
Copy link

@graceguo-supercat graceguo-supercat commented Aug 2, 2018

this is to fix issue #5532.

This PR covers following things:

  • remove DASHBOARD_ and _TYPE prefix/suffix in component ids (and db migration for saved data).
  • remove leading and trailing white spaces in stored position_json data
  • when user tries to save dashboard, check the size of position_json:
    • if larger than threshold, show error message and user can't save it
    • if size is larger than 90% of threshold, show warning message and user still can save it.
    • add dashboard config for dashboard position threshold, default is 65535.

@john-bodley @michellethomas @mistercrunch

'DASHBOARD_CHART_TYPE-ryxVc8RHlX': {
type: 'DASHBOARD_CHART_TYPE',
id: 'DASHBOARD_CHART_TYPE-ryxVc8RHlX',
ROOT_ID: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The first 3 lines seem to provide redundant information.

ROOT_ID: { // << key
  type: 'ROOT',  // same information that can be parsed from key
  id: 'ROOT_ID', // always same with key
  children: ['GRID_ID']
}

If we were to keep type as part of the ID, can we make id = ${type}_${identifier} a convention and get rid of type field in the object?

The id field value (e.g. ROOT_ID) is always equal to the key. Could we construct that if needed when parsing?

ROOT_ID: {
  children: ['GRID_ID']
},
GRID_ID: {
  children: ['CHART_123']
}.
CHART_123: {
  children: [] // this field seems redundant for leaf nodes. 
} 

or if these are stored as an array instead and converted to hash after parsing.

[
  { id: 'ROOT_ID', children: ['GRID_ID'] },
  { id: 'GRID_ID', children: ['CHART_123']},
  'CHART_123' // plain string when no children or other metadata
]

p.s. I haven't read the dashboard engine code so not sure how much change this will affect that code or are there other constraints? Just think that these approaches may save more space.

@graceguo-supercat
Copy link
Author

graceguo-supercat commented Aug 2, 2018

@kristw Thank you for all the good comments! Here are my explanations:

  • Keep an id: [key] property is a good practice for normalized redux state: https://redux.js.org/recipes/structuringreducers/normalizingstateshape. It did duplicate the entry key, but it gives us a lot of convenience when access/traverse the data itself.

  • Encode type information in id property is optional, i still want type as a mandatory property. Currently id has type information, but it is optional (for easier debug). In the worst case we can remove type from id. But most of code won't affected since we have type mandatory property.

  • remove children:[] from leave node is a good idea, but it also involved both frontend JS and more complicated migration code. I also feel in our code base we assume children property is required for all nodes. it required a lot tests before we can remove it.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! thanks for making this more concise!

text = text.replace('_TYPE', '')

dashboard.position_json = text
print('dash id:{} position_json size from {} to {}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you need the print? seems potentially useful

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah. i can see results from deployment log, kind of reduced 50% spaces...

@graceguo-supercat graceguo-supercat merged commit 2e2c980 into apache:master Aug 3, 2018
graceguo-supercat pushed a commit to graceguo-supercat/superset that referenced this pull request Aug 3, 2018
graceguo-supercat pushed a commit to airbnb/superset-fork that referenced this pull request Aug 3, 2018
…board

Reduce dashboard position_json data size (apache#5543)
@graceguo-supercat graceguo-supercat deleted the gg-ReduceDashboardPositionData branch August 6, 2018 23:36
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.28.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants