Skip to content

Conversation

@ivandika3
Copy link
Contributor

@ivandika3 ivandika3 commented Mar 8, 2024

What changes were proposed in this pull request?

Currently in the Datanode page the "Storage Capacity" columns only supports sorting by "storageRemaining". Add a filter option for user to be able to pick other storage metrics.

The options are:

  • Storage Remaining (default)
  • Storage Used
  • Storage Committed
  • Storage Total
  • Storage Utilization (include both Ozone and non-ozone usage): The one shown in the percentage of the storage bar

This patch will reuse the Antd table filter and uses the filter info state to set which storage metric the column will be sorted by (i.e. the filter will not be used for filtering data, but will be used as a placeholder to manage the sort state). You can refer to https://3x.ant.design/components/table/#components-table-demo-reset-filter for some of the implementation (See filteredInfo and onChange).

This requires some React state management which requires moving the columns definition into the React class, but the main change is only on the storageCapacity column.

Note: I tried setting defaultFilteredValue = ['storageRemaining'] and filteredValue = ['storageRemaining'] for the filter to pick the Storage Remaining by default, but defaultFilteredValue does not seem to work and filteredValue causes the filtered value to randomly get reset back to 'storageRemaining' although other filters are chosen. So currently I used the "(default)" in the filter label to pick which metric is default. Any suggestion is appreciated.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-10494

How was this patch tested?

Manual test using pnpm run dev.

Screenshot:

image

@ivandika3 ivandika3 marked this pull request as ready for review March 8, 2024 09:00
@ivandika3
Copy link
Contributor Author

ivandika3 commented Mar 8, 2024

@devabhishekpal @devmadhuu @dombizita @smitajoshi12 @ArafatKhan2198 Could you help review when you have time?

Copy link
Contributor

@devmadhuu devmadhuu left a comment

Choose a reason for hiding this comment

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

Thanks @ivandika3 for working on this patch. Few comments.


import React from 'react';
import {Table, Icon, Tooltip, Popover} from 'antd';
import {Icon, Popover, Table, Tooltip} from 'antd';
Copy link
Contributor

Choose a reason for hiding this comment

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

If no impact in ordering of imports, then kindly don't change this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for the catch. Seems it's automatically updated by the IDE. I have reverted it.

DatanodeStateList,
DatanodeOpState,
DatanodeOpStateList,
DatanodeState,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment here, If no impact in ordering of imports, then kindly don't change this line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

import {AutoReloadHelper} from 'utils/autoReloadHelper';
import AutoReloadPanel from 'components/autoReloadPanel/autoReloadPanel';
import {MultiSelect, IOption} from 'components/multiSelect/multiSelect';
import {IOption, MultiSelect} from 'components/multiSelect/multiSelect';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

import {getCapacityPercent, showDataFetchError} from 'utils/common';
import {ColumnSearch} from 'utils/columnSearch';
import { AxiosGetHelper } from 'utils/axiosRequestHelper';
import {AxiosGetHelper} from 'utils/axiosRequestHelper';
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, don't see any change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

},
sorter: (a: IDatanode, b: IDatanode) => {
let sortBy = "storageRemaining"
if (this.state.filteredInfo.storageCapacity &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Not clear with this part, whenever user tries to sort, here we are checking storageCapacity column length and has data, and if yes, then we are setting sortBy=first filter value always ? Am i missing something here ?

Copy link
Contributor Author

@ivandika3 ivandika3 Mar 11, 2024

Choose a reason for hiding this comment

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

Thank you for the review.

here we are checking storageCapacity column length and has data, and if yes, then we are setting sortBy=first filter value always ?

From the AntD table API documentation (https://3x.ant.design/components/table/#API).

image

The filter value has a type string[] and therefore the filtered info has the type Partial<Record<string, string[]>> (in IDatanodeState).

So when we change the filter value, the filtered info would be something like

Storage remaining

{
  "storageCapacity": ["storageRemaining"] 
}

Storage Used

{
  "storageCapacity": ["storageUsed"] 
}

The reason why the filter only contains 1 element is that filterMultiple for the 'Storage Capacity' column is set to false. If it's set to true, user can pick more than one filter and the array will be larger than 1.

Therefore, since at any point of time, at most one filter value can be picked, we can pick the first value of the array after doing the nullity check and length check. If the filter is not picked yet, we will pick the default storage metric (in this case storageRemaining)

Hopefully this clarifies.

@smitajoshi12
Copy link
Contributor

smitajoshi12 commented Mar 11, 2024

@ivandika3

  1. Can you give more brief regarding Mapping Storage Commited ,Storage Commited or How it is computed from Which API values in UI or may be in some tooltip

image

  1. Can you check with actual cluster data

@ivandika3
Copy link
Contributor Author

@smitajoshi12 Thank you for the review

  1. Can you give more brief regarding Mapping Storage Commited ,Storage Commited or How it is computed from Which API values in UI or may be in some tooltip

The "Storage Committed" was introduced recently in #5721. Our cluster has not backported this patch, but I put this in this patch just for completeness sake.

Reading the implementation, seems that committed bytes is used to represent unused bytes for a container. For example, if there is 1GB written in a 5GB container, the committed bytes would be 5GB - 1GB = 4GB. @vtutrinov @xichen01 maybe you can confirm.

Seems in #5721, only the tooltip is updated, but Progress component is not updated. Perhaps we can add the "storage committed" in the Progress component in future patch.

  1. Can you check with actual cluster data

Fix some incorrect calculation and tested against our internal cluster.

Copy link
Contributor

@dombizita dombizita left a comment

Choose a reason for hiding this comment

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

Thanks for this improvement @ivandika3, it looks good to me! I also tested it with pnpm.
One thing that came up while using it is that the filter names are different from the one that are showing up on the storage bar. I feel the ones you use are better, but maybe we could add tooltips to the filters, what do they mean. Correct me if I'm wrong, but:

  • Storage Remaining (default) is the Remaining in the storage bar - makes sense
  • Storage Used is Ozone Used - makes sense
  • Storage Committed is Container Pre-allocated - I could find this out from the code, we should change either of them and add some more information in a tooltip
  • Storage Total - makes sense
  • Storage Utilization - is this the total used storage?

It'd be the best if all the filters would be visible or easily connected to the parts of the storage bar.

@devmadhuu
Copy link
Contributor

@dombizita pls have a relook on this PR.

@ivandika3
Copy link
Contributor Author

@devmadhuu Thank you for checking this out again.

Let me try to address this in the future. Was working on it, but had to deal with some other tasks.

@dombizita I will let you know once I think it's ready for the subsequent review.

@ivandika3 ivandika3 added the UI label Apr 16, 2024
@ivandika3 ivandika3 self-assigned this Apr 16, 2024
@ivandika3
Copy link
Contributor Author

I'll abandon this for now. Feel free to pick this up if there is a need.

@ivandika3 ivandika3 closed this Oct 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants