[Canvas] Perf: save workbook assets separately from lighter weight structures #29594
Conversation
monfera
added
the
Team:Presentation
|
Pinging @elastic/kibana-canvas |
|
Something leaner or more orthogonal and future-proof could be whipped up (in this PR or later), but I thought it's better to have a checkpoint earlier. |
💚 Build Succeeded |
| server.route({ | ||
| method: 'PUT', | ||
| path: `${routePrefixStructures}/{id}`, | ||
| config: { payload: { allow: 'application/json', maxBytes: 26214400 } }, // 25MB payload limit |
There was a problem hiding this comment.
I don't think the workpad payload itself needs to have a 25MB limit, just the entire document (and it's mostly made up of the assets). It doesn't matter if we leave this here though, it shouldn't affect anything.
💚 Build Succeeded |
monfera
changed the title
| const changedWorkpad = workpadChanged(curState, newState); | ||
| const changedAssets = assetsChanged(curState, newState); | ||
|
|
||
| if (changedWorkpad && changedAssets) { |
There was a problem hiding this comment.
Interestingly, I can never hit this scenario. Which makes sense, all the state updates are sync and happen in order, and assets and workpads all have their own distinct actions.
There was a problem hiding this comment.
Yes, it's belts & braces, as the original code could've handled a common update and I shied away from proving it cannot happen. It'll probably be addressed on its own with the route simplification you suggested.
| return updateWorkpad(persistedWorkpad.id, persistedWorkpad).catch(notifyError); | ||
| } else if (changedAssets) { | ||
| // if the assets changed, save it to elasticsearch | ||
| const persistedWorkpad = getFullWorkpadPersisted(getState()); |
There was a problem hiding this comment.
nit: This is fine, but you could have just used getAssets from selectors/assets, which is how the workpad selector populates that value anyway.
| import { getId } from '../../public/lib/get_id'; | ||
|
|
||
| export function workpad(server) { | ||
| //const config = server.config(); | ||
| const { errors: esErrors } = server.plugins.elasticsearch.getCluster('data'); | ||
| const routePrefix = API_ROUTE_WORKPAD; | ||
| const routePrefixAssets = API_ROUTE_WORKPAD_ASSETS; | ||
| const routePrefixStructures = API_ROUTE_WORKPAD_STRUCTURES; |
There was a problem hiding this comment.
nit: I don't know if you explored this at all, but doing some kind of detection in a single route would be nice. Having 3 different routes to update a workpad (just the workpad, just the assets, or both) is easy, but seems like overkill. I'm fine leaving it this way though, just wanted to express that opinion.
There was a problem hiding this comment.
Created separate issue #29930 to address this and by extension, most of your other feedback items.
|
Thanks Joe! Merging it now, looking into the suggestions for a follow-up PR. |
…ructures (elastic#29594) * Perf: save workbook assets separately from lighter weight structures * Chore: simplification
Summary
Closes #28535