-
Notifications
You must be signed in to change notification settings - Fork 509
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
HDDS-10452. Improve Recon Disk Usage to fetch and display Top N records based on size. #6318
Conversation
@devmadhuu and @dombizita could you please take a look? |
@ArafatKhan2198 |
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 @ArafatKhan2198 for working on this patch. Few comments.
...ne/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryDiskUsageOrdering.java
Show resolved
Hide resolved
...ne/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryDiskUsageOrdering.java
Show resolved
Hide resolved
...ne/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryDiskUsageOrdering.java
Show resolved
Hide resolved
...ne/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryDiskUsageOrdering.java
Outdated
Show resolved
Hide resolved
@devmadhuu @adoroszlai @smitajoshi12 Could you please review the latest changes? Here's a quick summary:
|
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 for working on this @ArafatKhan2198, overall it looks good to me, I'd like to make the javadoc and comments more accurate, please see my comments inline.
hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/NSSummaryEndpoint.java
Outdated
Show resolved
Hide resolved
...zone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/BucketEntityHandler.java
Outdated
Show resolved
Hide resolved
...e/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/DirectoryEntityHandler.java
Outdated
Show resolved
Hide resolved
...-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/RootEntityHandler.java
Outdated
Show resolved
Hide resolved
...zone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/VolumeEntityHandler.java
Outdated
Show resolved
Hide resolved
Thanks @ArafatKhan2198 for handling some points. However I am not sure if parallelStreaming always improves performance, in fact rather sometimes, it increases more overhead and may do bad than good. I would like you to have a look here. |
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.
Some comments are still open. Pls handle them.
...ne/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryDiskUsageOrdering.java
Show resolved
Hide resolved
Thanks a lot, @devmadhuu , for the comment and the article! I've read through it carefully and here's my analysis: Parallel Streaming concern:
After going through the article I can summarise the following ➖
|
@ArafatKhan2198 @devmadhuu Please omit |
Do we have any performance measure data over 1 million records at least with and without parallel streaming. I am emphasizing it because I have experienced , that even with few 10K of records, parallel streaming do bad more than good. So I would suggest to publish some figures of performance with and without parallel streaming at least with 1 million records. |
Pls check on UI, what is the max limit in dropdown we are setting and using. I think its changed to 10k+. Pls check and confirm. |
Thanks for the comments @devmadhuu tested this out on a cluster with 10 million keys,
I believe we could got with parallel sort. |
Thanks @ArafatKhan2198 for testing out and publish the figures. This looks promising. |
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.
Changes LGTM +1. Pls resolve conflicts.
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.
A minor comment.
...zone/recon/src/main/java/org/apache/hadoop/ozone/recon/api/handlers/BucketEntityHandler.java
Show resolved
Hide resolved
def81a0
to
1d5a90f
Compare
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 @ArafatKhan2198 for working on this patch. Changes LGTM +1
@dombizita Could you please take a final look at it! |
...ne/recon/src/test/java/org/apache/hadoop/ozone/recon/api/TestNSSummaryDiskUsageOrdering.java
Show resolved
Hide resolved
Thanks @ArafatKhan2198 for working on this patch. |
…ds based on size. (apache#6318)
…ds based on size. (apache#6318) (cherry picked from commit 93a2489)
…ds based on size. (apache#6318) (cherry picked from commit 93a2489)
…ds based on size. (apache#6318) (cherry picked from commit 93a2489)
…ds based on size. (apache#6318) (cherry picked from commit 93a2489)
…ds based on size. (apache#6318) (cherry picked from commit 93a2489)
…ds based on size. (apache#6318) (cherry picked from commit 93a2489)
What changes were proposed in this pull request?
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10452
How was this patch tested?
Manually Tested Out the API and also using Integration Testing :-
Results from Manual Testing :-
100MB
,10MB
,1MB
&10KB
underdir-1