-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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
Reduce dashboard position_json data size #5543
Conversation
cbb1165
to
97dfe2d
Compare
97dfe2d
to
ba0fe69
Compare
'DASHBOARD_CHART_TYPE-ryxVc8RHlX': { | ||
type: 'DASHBOARD_CHART_TYPE', | ||
id: 'DASHBOARD_CHART_TYPE-ryxVc8RHlX', | ||
ROOT_ID: { |
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.
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.
@kristw Thank you for all the good comments! Here are my explanations:
|
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! thanks for making this more concise!
text = text.replace('_TYPE', '') | ||
|
||
dashboard.position_json = text | ||
print('dash id:{} position_json size from {} to {}' |
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.
do you need the print? seems potentially useful
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.
yeah. i can see results from deployment log, kind of reduced 50% spaces...
(cherry picked from commit 2e2c980)
…board Reduce dashboard position_json data size (apache#5543)
this is to fix issue #5532.
This PR covers following things:
DASHBOARD_
and_TYPE
prefix/suffix in component ids (and db migration for saved data).@john-bodley @michellethomas @mistercrunch