Tejaskriya commented on code in PR #9400:
URL: https://github.com/apache/ozone/pull/9400#discussion_r2603204093
##########
hadoop-ozone/dist/src/main/compose/common/grafana/dashboards/Ozone - SCM
Safemode.json:
##########
@@ -0,0 +1,649 @@
+{
+ "annotations": {
+ "list": [
+ {
+ "builtIn": 1,
+ "datasource": {
+ "type": "prometheus"
+ },
+ "enable": true,
+ "hide": true,
+ "iconColor": "rgba(0, 211, 255, 1)",
+ "name": "Annotations & Alerts",
+ "type": "dashboard"
+ }
+ ]
+ },
+ "editable": true,
+ "fiscalYearStartMonth": 0,
+ "graphTooltip": 0,
+ "id": 1,
+ "links": [],
+ "panels": [
+ {
+ "collapsed": true,
Review Comment:
This line makes it such that the row (or section) is collapsed, i.e., the
panels are not visible you would have to click on the row's heading for it to
open up and show the panels.
I would suggest this to be set to false.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/DataNodeSafeModeRule.java:
##########
@@ -71,9 +72,14 @@ protected boolean validate() {
@Override
protected void process(NodeRegistrationContainerReport reportsProto) {
- registeredDnSet.add(reportsProto.getDatanodeDetails().getID());
+ DatanodeID dnId = reportsProto.getDatanodeDetails().getID();
+ boolean added = registeredDnSet.add(dnId);
registeredDns = registeredDnSet.size();
+ if (added) {
+ getSafeModeMetrics().incCurrentRegisteredDatanodesCount();
Review Comment:
Could you add test coverage for this metric in
TestDataNodeSafeModeRule.java? In the existing tests itself you can just assert
the right value.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/SCMSafeModeManager.java:
##########
@@ -119,6 +119,7 @@ public SafeModeMetrics getSafeModeMetrics() {
private void emitSafeModeStatus() {
final SafeModeStatus safeModeStatus = status.get();
+ safeModeMetrics.setScmInSafeMode(safeModeStatus.isInSafeMode());
Review Comment:
Could you add some assert lines in the TestSCMSafeModeManager.java for this
metric for test coverage?
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/safemode/SafeModeMetrics.java:
##########
@@ -52,6 +52,11 @@ public class SafeModeMetrics {
private @Metric MutableCounterLong
currentPipelinesWithAtleastOneReplicaReportedCount;
+ @Metric private MutableGaugeLong scmInSafeMode;
Review Comment:
Could you please add a description for the metric? There are many instances
in the repo where you can find (`@Metric("explanation")`)
As we are converting boolean to numeric here, a description would be better
Also can we change it to `MutableGaugeInt` instead?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
