-
Notifications
You must be signed in to change notification settings - Fork 593
HDDS-13183. Create Cluster Capacity page UI. #9584
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
base: master
Are you sure you want to change the base?
Conversation
(cherry picked from commit cc5cf02)
devmadhuu
left a comment
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.
Thanks @devabhishekpal for the patch.
- I think , we decided to use datanode dropdown as combo of textbox and dropdown, where user can enter also to search in long list of drop down if needed.
- We need export option for datanodes card atleast.
- By default, datanodes in dropdown needs to be populated in descending order of pending deletion size.
- We should gray out the list of failed datanodes in dropdown as their pending deletion data is no longer useful for user to be displayed. This should be possible using css.
- How are we planning to handle the case when the API response will be having duplicate nodes with same name. Pls check the API behavior in cases when same name DNs joins the cluster and if API will have response because API response contains datanode names as well as UUID, and UUID is only being treated as unique.
hadoop-ozone/recon/src/main/resources/webapps/recon/ozone-recon-web/api/db.json
Show resolved
Hide resolved
priyeshkaratha
left a comment
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.
Thanks @devabhishekpal for working on the patch.
I have two comments regarding current UI implementation for showing DN pendingDeletion and related refreshing strategy. Please have a look on that.
...ne/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/pages/capacity/capacity.tsx
Show resolved
Hide resolved
...ne/recon/src/main/resources/webapps/recon/ozone-recon-web/src/v2/pages/capacity/capacity.tsx
Show resolved
Hide resolved
priyeshkaratha
left a comment
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.
Thanks @devabhishekpal for updating the patch. Overall changes LGTM
|
@adoroszlai I have added the commons-csv library from apache commons. |
adoroszlai
left a comment
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.
Thanks @devabhishekpal for the heads-up.
I have added the commons-csv library from apache commons. It would be great if you could take a look as well in case we have any existing library which serves the purpose.
dependency check failure has instructions on what needs to be done when adding new one.
| <commons-collections.version>4.4</commons-collections.version> | ||
| <commons-compress.version>1.27.1</commons-compress.version> | ||
| <commons-configuration2.version>2.12.0</commons-configuration2.version> | ||
| <commons-csv.version>1.10.0</commons-csv.version> |
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.
Can we use the latest version, 1.14.2? Otherwise dependabot will submit PR to update this anyway, so we can skip that step.
devmadhuu
left a comment
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.
Thanks @devabhishekpal for the patch. Kindly add unit and integration test for the code including new API end point. Also UI dev tests. And a small nit.
| return Response.status(Response.Status.ACCEPTED) | ||
| .entity(stream) | ||
| .type("text/csv") | ||
| .header("Content-Disposition", "attachment; filename=\"pending_deletion_stats.csv\"") |
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.
if pending deletion data is for all datanodes, then we can rename the csv as "pending_deletion_alldatanode_stats.csv"
|
[Usability issue] Its better to use grid (+ pagination or infinity scrolling and filtering/sorting/searching capabilities) with info for all data data nodes. In case of hundreds datanodes it would be hard to explore and pinpoint data nodes that cause probles. |
What changes were proposed in this pull request?
Create the Cluster Capacity page UI
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13183
How was this patch tested?
Patch was tested manually.