adoroszlai commented on code in PR #7522:
URL: https://github.com/apache/ozone/pull/7522#discussion_r1870823731
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/ScmBlockDeletingServiceMetrics.java:
##########
@@ -196,6 +227,119 @@ public long getNumBlockDeletionTransactionDataNodes() {
return numBlockDeletionTransactionDataNodes.value();
}
+ public Map<UUID, DatanodeCommandCounts> getNumCommandsDatanode() {
+ return numCommandsDatanode;
+ }
+
+ @Override
+ public void getMetrics(MetricsCollector metricsCollector, boolean all) {
+ MetricsRecordBuilder builder = metricsCollector.addRecord(SOURCE_NAME);
+ numBlockDeletionCommandSent.snapshot(builder, all);
+ numBlockDeletionCommandSuccess.snapshot(builder, all);
+ numBlockDeletionCommandFailure.snapshot(builder, all);
+ numBlockDeletionTransactionSent.snapshot(builder, all);
+ numBlockDeletionTransactionSuccess.snapshot(builder, all);
+ numBlockDeletionTransactionFailure.snapshot(builder, all);
+ numBlockDeletionTransactionCompleted.snapshot(builder, all);
+ numBlockDeletionTransactionCreated.snapshot(builder, all);
+ numSkippedTransactions.snapshot(builder, all);
+ numProcessedTransactions.snapshot(builder, all);
+ numBlockDeletionTransactionDataNodes.snapshot(builder, all);
+
+ MetricsRecordBuilder recordBuilder = builder;
+ for (Map.Entry<UUID, DatanodeCommandCounts> e :
numCommandsDatanode.entrySet()) {
+ recordBuilder = recordBuilder.endRecord().addRecord(SOURCE_NAME)
+ .add(new MetricsTag(Interns.info("datanode",
+ "Datanode host for deletion commands"), e.getKey().toString()))
+ .addGauge(DatanodeCommandCounts.COMMANDS_SENT_TO_DN,
+ e.getValue().getCommandsSent())
+ .addGauge(DatanodeCommandCounts.COMMANDS_SUCCESSFUL_EXECUTION_BY_DN,
+ e.getValue().getCommandsSuccess())
+ .addGauge(DatanodeCommandCounts.COMMANDS_FAILED_EXECUTION_BY_DN,
+ e.getValue().getCommandsFailure());
+ }
+ recordBuilder.endRecord();
+ }
+
+ /**
+ * Class contains metrics related to the ScmBlockDeletingService for each
datanode.
+ */
+ public static final class DatanodeCommandCounts {
+ private long commandsSent;
+ private long commandsSuccess;
+ private long commandsFailure;
+
+ private static final MetricsInfo COMMANDS_SENT_TO_DN = Interns.info(
+ "CommandsSent",
+ "Number of commands sent from SCM to the datanode for deletion");
+ private static final MetricsInfo COMMANDS_SUCCESSFUL_EXECUTION_BY_DN =
Interns.info(
+ "CommandsSuccess",
+ "Number of commands sent from SCM to the datanode for deletion for
which execution succeeded.");
+ private static final MetricsInfo COMMANDS_FAILED_EXECUTION_BY_DN =
Interns.info(
+ "CommandsFailed",
+ "Number of commands sent from SCM to the datanode for deletion for
which execution failed.");
+
+ public DatanodeCommandCounts() {
+ this.commandsSent = 0;
+ this.commandsSuccess = 0;
+ this.commandsFailure = 0;
+ }
+
+ public void incrCommandsSent(long delta) {
+ this.commandsSent += delta;
+ }
+
+ public void incrCommandsSuccess(long delta) {
+ this.commandsSuccess += delta;
+ }
+
+ public void incrCommandsFailure(long delta) {
+ this.commandsFailure += delta;
+ }
+
+ public long getCommandsSent() {
+ return commandsSent;
+ }
+
+ public long getCommandsSuccess() {
+ return commandsSuccess;
+ }
+
+ public long getCommandsFailure() {
+ return commandsFailure;
+ }
+
+ @Override
+ public String toString() {
+ return "Sent=" + commandsSent + ", Success=" + commandsSuccess + ",
Failed=" + commandsFailure;
+ }
+ }
+
+ @VisibleForTesting
+ public int getNumCommandsDatanodeSent() {
+ int sent = 0;
Review Comment:
Items being summed are `long`, this should be `long`, too.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/ScmBlockDeletingServiceMetrics.java:
##########
@@ -196,6 +227,119 @@ public long getNumBlockDeletionTransactionDataNodes() {
return numBlockDeletionTransactionDataNodes.value();
}
+ public Map<UUID, DatanodeCommandCounts> getNumCommandsDatanode() {
+ return numCommandsDatanode;
+ }
+
+ @Override
+ public void getMetrics(MetricsCollector metricsCollector, boolean all) {
+ MetricsRecordBuilder builder = metricsCollector.addRecord(SOURCE_NAME);
+ numBlockDeletionCommandSent.snapshot(builder, all);
+ numBlockDeletionCommandSuccess.snapshot(builder, all);
+ numBlockDeletionCommandFailure.snapshot(builder, all);
+ numBlockDeletionTransactionSent.snapshot(builder, all);
+ numBlockDeletionTransactionSuccess.snapshot(builder, all);
+ numBlockDeletionTransactionFailure.snapshot(builder, all);
+ numBlockDeletionTransactionCompleted.snapshot(builder, all);
+ numBlockDeletionTransactionCreated.snapshot(builder, all);
+ numSkippedTransactions.snapshot(builder, all);
+ numProcessedTransactions.snapshot(builder, all);
+ numBlockDeletionTransactionDataNodes.snapshot(builder, all);
+
+ MetricsRecordBuilder recordBuilder = builder;
+ for (Map.Entry<UUID, DatanodeCommandCounts> e :
numCommandsDatanode.entrySet()) {
+ recordBuilder = recordBuilder.endRecord().addRecord(SOURCE_NAME)
+ .add(new MetricsTag(Interns.info("datanode",
+ "Datanode host for deletion commands"), e.getKey().toString()))
+ .addGauge(DatanodeCommandCounts.COMMANDS_SENT_TO_DN,
+ e.getValue().getCommandsSent())
+ .addGauge(DatanodeCommandCounts.COMMANDS_SUCCESSFUL_EXECUTION_BY_DN,
+ e.getValue().getCommandsSuccess())
+ .addGauge(DatanodeCommandCounts.COMMANDS_FAILED_EXECUTION_BY_DN,
+ e.getValue().getCommandsFailure());
+ }
+ recordBuilder.endRecord();
+ }
+
+ /**
+ * Class contains metrics related to the ScmBlockDeletingService for each
datanode.
+ */
+ public static final class DatanodeCommandCounts {
+ private long commandsSent;
+ private long commandsSuccess;
+ private long commandsFailure;
+
+ private static final MetricsInfo COMMANDS_SENT_TO_DN = Interns.info(
+ "CommandsSent",
+ "Number of commands sent from SCM to the datanode for deletion");
+ private static final MetricsInfo COMMANDS_SUCCESSFUL_EXECUTION_BY_DN =
Interns.info(
+ "CommandsSuccess",
+ "Number of commands sent from SCM to the datanode for deletion for
which execution succeeded.");
+ private static final MetricsInfo COMMANDS_FAILED_EXECUTION_BY_DN =
Interns.info(
+ "CommandsFailed",
+ "Number of commands sent from SCM to the datanode for deletion for
which execution failed.");
+
+ public DatanodeCommandCounts() {
+ this.commandsSent = 0;
+ this.commandsSuccess = 0;
+ this.commandsFailure = 0;
+ }
+
+ public void incrCommandsSent(long delta) {
+ this.commandsSent += delta;
+ }
+
+ public void incrCommandsSuccess(long delta) {
+ this.commandsSuccess += delta;
+ }
+
+ public void incrCommandsFailure(long delta) {
+ this.commandsFailure += delta;
+ }
+
+ public long getCommandsSent() {
+ return commandsSent;
+ }
+
+ public long getCommandsSuccess() {
+ return commandsSuccess;
+ }
+
+ public long getCommandsFailure() {
+ return commandsFailure;
+ }
+
+ @Override
+ public String toString() {
+ return "Sent=" + commandsSent + ", Success=" + commandsSuccess + ",
Failed=" + commandsFailure;
+ }
+ }
+
+ @VisibleForTesting
Review Comment:
nit: I don't think we need this annotation. It is usually applied to
methods that do some internal operation, and are exposed only for tests. These
`getNum...()` methods exists only for tests, which is different. They don't
mutate internal state or do anything "dangerous" that should not be in
production code. So I think having such safe public methods without adding the
annotation is OK.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/ScmBlockDeletingServiceMetrics.java:
##########
@@ -196,6 +227,119 @@ public long getNumBlockDeletionTransactionDataNodes() {
return numBlockDeletionTransactionDataNodes.value();
}
+ public Map<UUID, DatanodeCommandCounts> getNumCommandsDatanode() {
+ return numCommandsDatanode;
+ }
+
+ @Override
+ public void getMetrics(MetricsCollector metricsCollector, boolean all) {
+ MetricsRecordBuilder builder = metricsCollector.addRecord(SOURCE_NAME);
+ numBlockDeletionCommandSent.snapshot(builder, all);
+ numBlockDeletionCommandSuccess.snapshot(builder, all);
+ numBlockDeletionCommandFailure.snapshot(builder, all);
+ numBlockDeletionTransactionSent.snapshot(builder, all);
+ numBlockDeletionTransactionSuccess.snapshot(builder, all);
+ numBlockDeletionTransactionFailure.snapshot(builder, all);
+ numBlockDeletionTransactionCompleted.snapshot(builder, all);
+ numBlockDeletionTransactionCreated.snapshot(builder, all);
+ numSkippedTransactions.snapshot(builder, all);
+ numProcessedTransactions.snapshot(builder, all);
+ numBlockDeletionTransactionDataNodes.snapshot(builder, all);
+
+ MetricsRecordBuilder recordBuilder = builder;
+ for (Map.Entry<UUID, DatanodeCommandCounts> e :
numCommandsDatanode.entrySet()) {
+ recordBuilder = recordBuilder.endRecord().addRecord(SOURCE_NAME)
+ .add(new MetricsTag(Interns.info("datanode",
+ "Datanode host for deletion commands"), e.getKey().toString()))
+ .addGauge(DatanodeCommandCounts.COMMANDS_SENT_TO_DN,
+ e.getValue().getCommandsSent())
+ .addGauge(DatanodeCommandCounts.COMMANDS_SUCCESSFUL_EXECUTION_BY_DN,
+ e.getValue().getCommandsSuccess())
+ .addGauge(DatanodeCommandCounts.COMMANDS_FAILED_EXECUTION_BY_DN,
+ e.getValue().getCommandsFailure());
+ }
+ recordBuilder.endRecord();
+ }
+
+ /**
+ * Class contains metrics related to the ScmBlockDeletingService for each
datanode.
+ */
+ public static final class DatanodeCommandCounts {
+ private long commandsSent;
+ private long commandsSuccess;
+ private long commandsFailure;
+
+ private static final MetricsInfo COMMANDS_SENT_TO_DN = Interns.info(
+ "CommandsSent",
+ "Number of commands sent from SCM to the datanode for deletion");
+ private static final MetricsInfo COMMANDS_SUCCESSFUL_EXECUTION_BY_DN =
Interns.info(
+ "CommandsSuccess",
+ "Number of commands sent from SCM to the datanode for deletion for
which execution succeeded.");
+ private static final MetricsInfo COMMANDS_FAILED_EXECUTION_BY_DN =
Interns.info(
+ "CommandsFailed",
+ "Number of commands sent from SCM to the datanode for deletion for
which execution failed.");
+
+ public DatanodeCommandCounts() {
+ this.commandsSent = 0;
+ this.commandsSuccess = 0;
+ this.commandsFailure = 0;
+ }
+
+ public void incrCommandsSent(long delta) {
+ this.commandsSent += delta;
+ }
+
+ public void incrCommandsSuccess(long delta) {
+ this.commandsSuccess += delta;
+ }
+
+ public void incrCommandsFailure(long delta) {
+ this.commandsFailure += delta;
+ }
+
+ public long getCommandsSent() {
+ return commandsSent;
+ }
+
+ public long getCommandsSuccess() {
+ return commandsSuccess;
+ }
+
+ public long getCommandsFailure() {
+ return commandsFailure;
+ }
+
+ @Override
+ public String toString() {
+ return "Sent=" + commandsSent + ", Success=" + commandsSuccess + ",
Failed=" + commandsFailure;
+ }
+ }
+
+ @VisibleForTesting
+ public int getNumCommandsDatanodeSent() {
+ int sent = 0;
+ for (Map.Entry<UUID, DatanodeCommandCounts> e :
numCommandsDatanode.entrySet()) {
+ sent += e.getValue().commandsSent;
Review Comment:
Iterate `values()` instead of `entrySet()`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/ScmBlockDeletingServiceMetrics.java:
##########
@@ -86,7 +100,11 @@ public final class ScmBlockDeletingServiceMetrics {
@Metric(about = "The number of dataNodes of delete transactions.")
private MutableGaugeLong numBlockDeletionTransactionDataNodes;
+ private Map<UUID, DatanodeCommandCounts> numCommandsDatanode;
Review Comment:
Can be `final`.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/ScmBlockDeletingServiceMetrics.java:
##########
@@ -86,7 +100,11 @@ public final class ScmBlockDeletingServiceMetrics {
@Metric(about = "The number of dataNodes of delete transactions.")
private MutableGaugeLong numBlockDeletionTransactionDataNodes;
+ private Map<UUID, DatanodeCommandCounts> numCommandsDatanode;
+
private ScmBlockDeletingServiceMetrics() {
+ this.registry = new MetricsRegistry(SOURCE_NAME);
+ numCommandsDatanode = new HashMap<>();
Review Comment:
Please use `ConcurrentHashMap` for thread safety.
--
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]
