Workloads overview: don't block on store load #1829
Conversation
jakolehm
commented
Dec 21, 2020
jakolehm
commented
- renders overview immediately (and updates graphs on-demand), no need to wait for all related stores to be fully loaded
- removes concurrent load of stores, this causes too much load on k8s api on big clusters
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
| for (const store of stores) { | ||
| await store.loadAll(); | ||
| } |
There was a problem hiding this comment.
This might lead to memory leaks and/or errors IMHO.
It creates sequential loading for all stores, so the code after for-cycle might be evaluated when user already has left current page (but stores still loading data). For big clusters could be real-case scenario?
There was a problem hiding this comment.
Or actually I might be wrong.. since unsubscribing would happen anyway:
await when(() => this.isUnmounting);There was a problem hiding this comment.
But still for big clusters could be a traffic issue since it's continue to load all stores even if user left the page.
There was a problem hiding this comment.
Probably not worse than the current implementation which triggers all requests in parallel?
| for (const store of stores) { | ||
| await store.loadAll(); | ||
| } |
There was a problem hiding this comment.
Or actually I might be wrong.. since unsubscribing would happen anyway:
await when(() => this.isUnmounting);
| @@ -24,7 +23,6 @@ interface Props extends RouteComponentProps<IWorkloadsOverviewRouteParams> { | |||
|
|
|||
| @observer | |||
| export class WorkloadsOverview extends React.Component<Props> { | |||
| @observable isReady = false; | |||
| @observable isUnmounting = false; | |||
There was a problem hiding this comment.
I think that this is the wrong way to have this, it should be a list of disposers which is called in the componentWillUnmount function.
It is useful to still have this variable but I don't think it should be observable since it is only read from componentDidMount and only written to in componentWillUnmount.
There was a problem hiding this comment.
The problem with componentWillUnmount that it might not work as expected cause of async componentDidMount.. So this is better solution in that case..
Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
|
@nevalla @ixrock @aleksfront ptal |
* workloads overview: don't block on store load Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com> * subscribe after loadAll Signed-off-by: Jari Kolehmainen <jari.kolehmainen@gmail.com>
