-
Notifications
You must be signed in to change notification settings - Fork 490
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-11206. Statistics storage usage indicators include min, max, median, stdev #6977
Conversation
Can you help review this PR, @sumitagrawl @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.
Thank for the patch @jianghuazhu , left a few comments.
@@ -48,4 +49,8 @@ public interface NodeManagerMXBean { | |||
*/ | |||
Map<String, Map<String, String>> getNodeStatusInfo(); | |||
|
|||
default Map<String, String> getNodeStatics() { |
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.
Typo: Statics
-> Statistics
.
Also applies to other instances of Statics
in the patch.
private enum UsageStatics { | ||
MIN("Min"), | ||
MAX("Max"), | ||
MEDINNA("Medina"), |
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.
Typo: MEDINA
-> MEDIAN
. Also applies to similar typos.
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 your comment and review, @ivandika3 .
I'll update later.
hadoop-hdds/server-scm/src/main/resources/webapps/scm/scm-overview.html
Outdated
Show resolved
Hide resolved
$scope.statistical = { | ||
usages : {} | ||
} | ||
$scope.statistical.usages.min = ""; | ||
$scope.statistical.usages.max = ""; | ||
$scope.statistical.usages.medina = ""; | ||
$scope.statistical.usages.stdev = ""; |
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.
Nit: statistical
-> statistics
.
I think it's also better to be more specific that it is the statistics for DNs (since technically we can have statistics for SCM in the future). Maybe scope.statistics.nodes.usages.
prefix is better.
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.
I have updated it.
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 updating the patch. LGTM +1.
Map<String, String> nodeStatics = new HashMap<>(); | ||
// Statistics node usaged | ||
nodeUsageStatics(nodeStatics); |
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.
Nit: Statics
-> Stats
/ Statistics
@jianghuazhu Can you please update the PR description with the latest screenshots. |
Hello @jianghuazhu, thank you for this patch. Statistical
to Statistics
|
Thanks for your comments and reviews, @ArafatKhan2198 @devabhishekpal . |
$scope.statistics = { | ||
nodes : { | ||
usages : {} | ||
} | ||
} | ||
$scope.statistics.nodes.usages.min = ""; | ||
$scope.statistics.nodes.usages.max = ""; | ||
$scope.statistics.nodes.usages.median = ""; | ||
$scope.statistics.nodes.usages.stdev = ""; |
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.
Hi @jianghuazhu, was there any reason behind separating these values out?
I believe we can declare them in the statistics object.
Also I think initializing them to something like "N/A" would be better as in case there is any error, the default value won't be empty but rather show something to let the user know something went wrong.
$scope.statistics = { | |
nodes : { | |
usages : {} | |
} | |
} | |
$scope.statistics.nodes.usages.min = ""; | |
$scope.statistics.nodes.usages.max = ""; | |
$scope.statistics.nodes.usages.median = ""; | |
$scope.statistics.nodes.usages.stdev = ""; | |
$scope.statistics = { | |
nodes : { | |
usages : { | |
min: "N/A", | |
max: "N/A", | |
median: "N/A", | |
stdev: "N/A" | |
} | |
} | |
} |
@@ -81,6 +90,18 @@ | |||
$scope.totalItems = nodeStatusCopy.length; | |||
$scope.lastIndex = Math.ceil(nodeStatusCopy.length / $scope.RecordsToDisplay); | |||
$scope.nodeStatus = nodeStatusCopy.slice(0, $scope.RecordsToDisplay); | |||
|
|||
ctrl.nodemanagermetrics.NodeStatistics.map(({ key, value }) => { |
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.
While using a .map()
will give us the same result, but it will also return an array (empty in this case) which we are ignoring as the value is not being assigned.
This might not be affecting the performance or anything, as it is a minor operation, but I would suggest to use .forEach
in this scenario.
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 @jianghuazhu for your changes.
Just a few minor comments, and then it looks good to me.
@@ -81,6 +91,18 @@ | |||
$scope.totalItems = nodeStatusCopy.length; | |||
$scope.lastIndex = Math.ceil(nodeStatusCopy.length / $scope.RecordsToDisplay); | |||
$scope.nodeStatus = nodeStatusCopy.slice(0, $scope.RecordsToDisplay); | |||
|
|||
ctrl.nodemanagermetrics.NodeStatistics.forEach(function(obj) { |
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.
Just fyi, can de-structure this as well like you did for map().
Since obj = { key: abc, value: xyz }, we can destructure it as:
{key, value} = obj
So the function could also be written as:
ctrl.nodemanagermetrics.NodeStatistics.forEach(({key, value}) => {
....
});
But your current change is good as well
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 a lot @jianghuazhu for your changes, this addition is great.
Thank you @ivandika3 for reviewing this PR
The changes LGTM, +1
Thanks for your comments and reviews, @devabhishekpal , @ivandika3 . |
Thank you for the patch @jianghuazhu, and @devabhishekpal @ArafatKhan2198 for the reviews. |
What changes were proposed in this pull request?
For SCM, it is very useful to count the min, max, median, and stdev of node storage.
![image](https://private-user-images.githubusercontent.com/6416939/352489280-b675c4c3-6368-4d1b-9893-2c0ac6978965.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjUyODg1NzIsIm5iZiI6MTcyNTI4ODI3MiwicGF0aCI6Ii82NDE2OTM5LzM1MjQ4OTI4MC1iNjc1YzRjMy02MzY4LTRkMWItOTg5My0yYzBhYzY5Nzg5NjUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwMiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDJUMTQ0NDMyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9NWU4MWE0ODNjY2FhODRkY2RhNWRiMjdjODE2NzAxZDE2MjQ0YWZlNDk1Y2NkZDVmYjQyNzhhOGRiOWRiZjdiYiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.F_LPDluOiMRWk_UZO5i5cu2HU1YEYAfeDpEvB_JF4o0)
After adding the patch:
jmx:
ui:
![image](https://private-user-images.githubusercontent.com/6416939/352489365-42809667-bd4b-4587-8667-88fb88995663.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjUyODg1NzIsIm5iZiI6MTcyNTI4ODI3MiwicGF0aCI6Ii82NDE2OTM5LzM1MjQ4OTM2NS00MjgwOTY2Ny1iZDRiLTQ1ODctODY2Ny04OGZiODg5OTU2NjMucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI0MDkwMiUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNDA5MDJUMTQ0NDMyWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9OWUzZjNmMzZiMTFiNTA3NTgwNmMwNWUyMDgzMDI0ZDBjNWZjMjAxMWNkMjI2NTE0NGE3ZGRjYTMwNzFiMDlhMSZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QmYWN0b3JfaWQ9MCZrZXlfaWQ9MCZyZXBvX2lkPTAifQ.YXFZwIEa8yPR7lthYJCvdo11nXf3x3pzfrfWKeuXEhM)
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-11206
How was this patch tested?
It is necessary to ensure that jmx can display NodeStatics correctly.
It is necessary to ensure that the UI can display Statistical correctly.