Replace StatsUpdater with multi-cluster aware ClusterHealthManager #374
Conversation
|
I see tests are failing. I don't have time to fix those today. @alexdametto as you are away on Monday, please sanity check this PR today if you have time so I can fix up the tests and any issues you find on Monday. Thank you. |
| @@ -39,7 +40,7 @@ function App() { | |||
|
|
|||
| return ( | |||
| <GcAdminAntdProvider> | |||
| <StatsUpdater /> | |||
| <ClusterHealthManager clusterId="" /> | |||
There was a problem hiding this comment.
I think here you are missing the clusterId prop.
Note: another approach could be removing the parameter into the component and the component can get the cluster id from the context itself. Fine for me both ways.
There was a problem hiding this comment.
Oh but maybe the clusterId prop is needed in cloud-ui to allow pre-fetch / multiple clusters, in that case ignore the comment above
| const { data: nodes } = useClusterNodeStatus(clusterId); | ||
| const [prevNodeData, setPrevNodesData] = useState<NodeStatusInfo[] | []>(); | ||
|
|
||
| const getFs = (nodes: NodeStatusInfo[]): { [key: string]: FSStats } => { |
There was a problem hiding this comment.
I think that
| const getFs = (nodes: NodeStatusInfo[]): { [key: string]: FSStats } => { | |
| const getFs = (nodes: NodeStatusInfo[]): Record<string, FSStats> => { |
is more readable
| fsStats: { [key: string]: FSStats }; | ||
|
|
||
| // health | ||
| clusterHealth: { [clusterId: string]: ClusterHealth }; |
There was a problem hiding this comment.
| clusterHealth: { [clusterId: string]: ClusterHealth }; | |
| clusterHealth: Record<string, ClusterHealth>; |
Summary of changes
☝️ In short, the old
StatsUpdaterworked, but was only aware of one cluster. This means when used incloud-ui, and switching between clusters, cluster A's stats were mixed with cluster B's meaning none of the numbers are correct.cloud-uitoday. Stats would require the cluster being JWT compatible, however.ClusterHealthManagerbut there were none forStatsUpdatereither. I think writing tests here will be very difficult as the component exports zero output. I am happy to try but I am not sure it is worth the effort at this time.Checklist
CHANGES.md.