Skip to content

HDDS-11017. Migrated to ECharts, Vite and AntD v4 with eslint, prettier#6841

Merged
ArafatKhan2198 merged 19 commits intoapache:masterfrom
devabhishekpal:HDDS-11017
Jul 10, 2024
Merged

HDDS-11017. Migrated to ECharts, Vite and AntD v4 with eslint, prettier#6841
ArafatKhan2198 merged 19 commits intoapache:masterfrom
devabhishekpal:HDDS-11017

Conversation

@devabhishekpal
Copy link
Contributor

What changes were proposed in this pull request?

  • While conducting a fossa scan we found that node-notifier and jsdom is not compatible with Apache license
  • This PR migrates from the react-scripts used in the current project to Vite and Vitest which resolved the licensing issue
  • Apart from that this will bring additional improvements like:
    • Faster local development time
    • Faster builds
    • Migration to AntD v5 which will allow us to directly upgrade to newer versions
    • Shift from xo to eslint and prettier - this will make the development experience more smooth and consistent for contributors

What is the link to the Apache JIRA

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

How was this patch tested?

The patch was tested manually on local build
Overview Page
Volumes Page
Volumes with ACL drawer
Buckets Page
Buckets with ACL Drawer
Datanodes Page
Pipelines Page
Containers Page
Insights File Size dist page
File Size Dist full chart
Ingihts Container Size Distribution
OM DB Insights Page
DU Page
Heatmap default path
Heatmap with buckets and changed path

@adoroszlai adoroszlai added dependencies Pull requests that update a dependency file recon UI build Pull request that modifies the build process labels Jun 21, 2024
@devabhishekpal devabhishekpal changed the title HDDS-11017. Migrated to ECharts, Vite and AntD v5 with eslint, prettier HDDS-11017. Migrated to ECharts, Vite and AntD v4 with eslint, prettier Jun 24, 2024
@devabhishekpal
Copy link
Contributor Author

Currently library selection no longer has any licensing issues.

@smitajoshi12
Copy link
Contributor

@devabhishekpal
Can we update node and pnpm version also?

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 @devabhishekpal for this effort and patch. Changes LGTM+1. Just a nit from my side.Request @dombizita to have a look.

@devabhishekpal
Copy link
Contributor Author

Hi @smitajoshi12 , so for Node v18 - it no longer supports CentOS 7 and older linux distros.
We would need to then specify it that the project cannot be built on older linux dist.
Hence we are stuck with Node v16 for the time being.
pnpm v9 required Node 18. So I am upgrading to the latest supported pnpm v8.15.7.

@smitajoshi12
Copy link
Contributor

@devabhishekpal
In Disk Usage CSS aligmnet has been changed for Show Metata Data button.

image

@smitajoshi12
Copy link
Contributor

@devabhishekpal

Can we move pi chart alignment to left side.

image

@smitajoshi12
Copy link
Contributor

@devabhishekpal

Css Issue in heatmap when selecting dropdown

image

@devabhishekpal
Copy link
Contributor Author

Hi @smitajoshi12, for #6841 (comment), if we move the pie-chart to the left side it would look odd as without the metadata panel open, it would have lots of empty space on the right hand side.
So we are keeping it in the center as when the metadata is viewed the user will either way not be interacting with the chart.

Please do let me know if you want to shift to the left or if you have any other alternate inputs.
Thanks

@smitajoshi12
Copy link
Contributor

Hi @smitajoshi12, for #6841 (comment), if we move the pie-chart to the left side it would look odd as without the metadata panel open, it would have lots of empty space on the right hand side. So we are keeping it in the center as when the metadata is viewed the user will either way not be interacting with the chart.

Please do let me know if you want to shift to the left or if you have any other alternate inputs. Thanks

@ivandika3 @dombizita
Can suggest but if entity name is lengthy then meta data summary pop is overlapping or we can mimimize metadata summary pop up so we will get clear visibility of entity names.

@devabhishekpal
Copy link
Contributor Author

@smitajoshi12 could you let me know the screen resolution you are using for #6841 (comment)?
It seems in my resolution this issue is not present.

Screen.Recording.2024-06-25.at.12.24.26.mp4

It is good to have other resolutions to test as well. Thanks a lot for bringing this to my attention.

@smitajoshi12
Copy link
Contributor

smitajoshi12 commented Jun 25, 2024

With Same Cluster data results are different in pi chart

filecount

[ {
"volume": "volume1",
"bucket": "bucket1",
"fileSize": 32768,
"count": 5
},{
"volume": "volume1",
"bucket": "bucket2",
"fileSize": 32768,
"count": 3
}
]


container count

[{"containerSize":536870912,"count":1}]

Old Library
image

image

New Library
image

image

@devabhishekpal
Copy link
Contributor Author

devabhishekpal commented Jun 25, 2024

Hi @smitajoshi12, thanks for pointing out this issue.
Fixed it in the latest patch 5db74c1
With your data now it is looking like below:
Screenshot 2024-06-25 at 18 00 32

One change from the original is that we are referring to the range instead of the actual value as in the original it seemed like multiple entities were having the same fixed container size.
Showing the range let's the user know that it is not a fixed size but a range in which the entities are spread

@smitajoshi12
Copy link
Contributor

smitajoshi12 commented Jun 27, 2024

@devabhishekpal
In Overview Page
In Route.json entries are also missing for local run getting 404 in local.

image

@smitajoshi12
Copy link
Contributor

@devabhishekpal

Looks like Container ID column alignment has been changed.

image

@smitajoshi12
Copy link
Contributor

smitajoshi12 commented Jun 27, 2024

@devabhishekpal

Can we increase width and Height of Pi Chart align more to left side so can get visibility for all entities.

image

@devabhishekpal
Copy link
Contributor Author

@devabhishekpal In Overview Page In Route.json entries are also missing for local run getting 404 in local.

image

Hi @smitajoshi12 , I checked the older routes.json as well. I mean from older branch (Ozone 1.4.0) it seems the path mapping to /triggerdbsync/om was never present, hence the 404 response.
But if we test it on a cluster instead of in dev mode, the API is being called. So it is okay in built files.

@smitajoshi12
Copy link
Contributor

@devabhishekpal In Overview Page In Route.json entries are also missing for local run getting 404 in local.
image

Hi @smitajoshi12 , I checked the older routes.json as well. I mean from older branch (Ozone 1.4.0) it seems the path mapping to /triggerdbsync/om was never present, hence the 404 response. But if we test it on a cluster instead of in dev mode, the API is being called. So it is okay in built files.

No Issues closing this comment

@devabhishekpal
Copy link
Contributor Author

@devabhishekpal

Looks like Container ID column alignment has been changed.

image

It seems this is the default behaviour with expandable rows.
This is also present in the existing code, I cross checked with the current code.
The following is built on master

Screenshot 2024-06-27 at 17 16 46

@devabhishekpal
Copy link
Contributor Author

@devabhishekpal

Can we increase width and Height of Pi Chart align more to left side so can get visibility for all entities.

image

So even if we increase the pie-chart width the percentage of entity area will be the same.
I added the changes to spread out the pie-chart and added padding to the Tab panels as you suggested.

However with a larger pie-chart the labels will overlap on the chart as seen below.
Screenshot 2024-06-27 at 18 37 10

So sticking to current size. I believe your PR to add a normalized width to the small entities would solve this issue of small areas.

@smitajoshi12
Copy link
Contributor

It looks good to me now.

@smitajoshi12
Copy link
Contributor

@devabhishekpal

Response and Pi Chart are different

image

Response:-
{
"status": "OK",
"path": "/vol1/bucket1/dir1/key1",
"size": 3800,
"sizeWithReplica": 11400,
"subPathCount": 0,
"subPaths": [],
"sizeDirectKey": 0
}

@smitajoshi12
Copy link
Contributor

@devabhishekpal

Response and Pi Chart are different

image

Response:- { "status": "OK", "path": "/vol1/bucket1/dir1/key1", "size": 3800, "sizeWithReplica": 11400, "subPathCount": 0, "subPaths": [], "sizeDirectKey": 0 }

It looks Good to me after your recent changes

@ArafatKhan2198
Copy link
Contributor

ArafatKhan2198 commented Jul 9, 2024

Thanks for the patch @devabhishekpal
Smitha recently merged a change in upstream, which brings normalisation to the pie charts of disk usage!
Have we tested out our new UI changes to the on it?

@devabhishekpal
Copy link
Contributor Author

Hi @ArafatKhan2198, yes - the changes have been tested with the the datasets that were used by @smitajoshi12 in the original normalization PR and it is working correctly.
The commit d8cdd92 adds the changes for the normalization.

Copy link
Contributor

@ArafatKhan2198 ArafatKhan2198 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 the patch @devabhishekpal
LGTM +1
Thanks for improving the UI for recon :)

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 @devabhishekpal . New changes LGTM +1

@ArafatKhan2198
Copy link
Contributor

Thanks for the work @devabhishekpal & thanks @devmadhuu @smitajoshi12 @ivandika3 @adoroszlai for the review.

@ArafatKhan2198 ArafatKhan2198 merged commit 89494f1 into apache:master Jul 10, 2024
@devabhishekpal devabhishekpal deleted the HDDS-11017 branch July 11, 2024 07:31
adoroszlai pushed a commit to adoroszlai/ozone that referenced this pull request Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Pull request that modifies the build process dependencies Pull requests that update a dependency file recon UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants