szetszwo commented on code in PR #9600:
URL: https://github.com/apache/ozone/pull/9600#discussion_r2670142955
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -34,47 +35,56 @@
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public class ProtocolMessageMetrics<KEY extends Enum<KEY>> implements
MetricsSource {
private final String name;
private final String description;
- private final Map<KEY, AtomicLong> counters =
- new ConcurrentHashMap<>();
-
- private final Map<KEY, AtomicLong> elapsedTimes =
- new ConcurrentHashMap<>();
+ private final Map<KEY, Stats> stats;
private final AtomicInteger concurrency = new AtomicInteger(0);
- public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
+ private static final MetricsInfo TYPE_TAG_INFO =
+ Interns.info("type", "Message type");
+
+ private static final MetricsInfo COUNTER_INFO =
+ Interns.info("counter", "Number of distinct calls");
+
+ private static final MetricsInfo TIME_INFO =
+ Interns.info("time", "Sum of the duration of the calls");
+
+ private static final MetricsInfo CONCURRENCY_INFO =
+ Interns.info("concurrency",
+ "Number of requests processed concurrently");
Review Comment:
The line width is 120 characters. Let's avoid the short lines.
```java
private static final MetricsInfo TYPE_TAG_INFO = Interns.info("type",
"Message type");
private static final MetricsInfo COUNTER_INFO = Interns.info("counter",
"Number of distinct calls");
private static final MetricsInfo TIME_INFO = Interns.info("time", "Sum of
the duration of the calls");
private static final MetricsInfo CONCURRENCY_INFO =
Interns.info("concurrency",
"Number of requests processed concurrently");
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -89,44 +99,41 @@ public void unregister() {
@Override
public void getMetrics(MetricsCollector collector, boolean all) {
- counters.forEach((key, value) -> {
+ stats.forEach((key, stat) -> {
MetricsRecordBuilder builder =
collector.addRecord(name);
builder.add(
- new MetricsTag(Interns.info("type", "Message type"),
key.toString()));
- builder.addCounter(new MetricName("counter", "Number of distinct calls"),
- value.longValue());
+ new MetricsTag(TYPE_TAG_INFO, key.toString()));
+ builder.addCounter(COUNTER_INFO,
+ stat.counter());
builder.addCounter(
- new MetricName("time", "Sum of the duration of the calls"),
- elapsedTimes.get(key).longValue());
+ TIME_INFO,
+ stat.time());
builder.endRecord();
Review Comment:
Let's combine the short lines:
```java
collector.addRecord(name)
.add(new MetricsTag(TYPE_TAG_INFO, key.toString()))
.addCounter(COUNTER_INFO, stat.counter())
.addCounter(TIME_INFO, stat.time())
.endRecord();
```
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -34,47 +35,56 @@
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public class ProtocolMessageMetrics<KEY extends Enum<KEY>> implements
MetricsSource {
Review Comment:
👍 the generic type should extend Enum.
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -89,44 +99,41 @@ public void unregister() {
@Override
public void getMetrics(MetricsCollector collector, boolean all) {
- counters.forEach((key, value) -> {
+ stats.forEach((key, stat) -> {
MetricsRecordBuilder builder =
collector.addRecord(name);
builder.add(
- new MetricsTag(Interns.info("type", "Message type"),
key.toString()));
- builder.addCounter(new MetricName("counter", "Number of distinct calls"),
- value.longValue());
+ new MetricsTag(TYPE_TAG_INFO, key.toString()));
+ builder.addCounter(COUNTER_INFO,
+ stat.counter());
builder.addCounter(
- new MetricName("time", "Sum of the duration of the calls"),
- elapsedTimes.get(key).longValue());
+ TIME_INFO,
+ stat.time());
builder.endRecord();
});
MetricsRecordBuilder builder = collector.addRecord(name);
- builder.addCounter(new MetricName("concurrency",
- "Number of requests processed concurrently"), concurrency.get());
+ builder.addCounter(CONCURRENCY_INFO, concurrency.get());
Review Comment:
Similarly,
```java
collector.addRecord(name)
.addCounter(CONCURRENCY_INFO, concurrency.get());
```
BTW, do we need `.endRecord();`?
##########
hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/ProtocolMessageMetrics.java:
##########
@@ -34,47 +35,56 @@
/**
* Metrics to count all the subtypes of a specific message.
*/
-public class ProtocolMessageMetrics<KEY> implements MetricsSource {
+public class ProtocolMessageMetrics<KEY extends Enum<KEY>> implements
MetricsSource {
private final String name;
private final String description;
- private final Map<KEY, AtomicLong> counters =
- new ConcurrentHashMap<>();
-
- private final Map<KEY, AtomicLong> elapsedTimes =
- new ConcurrentHashMap<>();
+ private final Map<KEY, Stats> stats;
private final AtomicInteger concurrency = new AtomicInteger(0);
- public static <KEY> ProtocolMessageMetrics<KEY> create(String name,
+ private static final MetricsInfo TYPE_TAG_INFO =
+ Interns.info("type", "Message type");
+
+ private static final MetricsInfo COUNTER_INFO =
+ Interns.info("counter", "Number of distinct calls");
+
+ private static final MetricsInfo TIME_INFO =
+ Interns.info("time", "Sum of the duration of the calls");
+
+ private static final MetricsInfo CONCURRENCY_INFO =
+ Interns.info("concurrency",
+ "Number of requests processed concurrently");
+
+ public static <KEY extends Enum<KEY>> ProtocolMessageMetrics<KEY>
create(String name,
String description, KEY[] types) {
- return new ProtocolMessageMetrics<KEY>(name, description, types);
+ return new ProtocolMessageMetrics<>(name, description, types);
}
public ProtocolMessageMetrics(String name, String description,
KEY[] values) {
this.name = name;
this.description = description;
+ final Class<KEY> enumClass = values[0].getDeclaringClass();
+ final EnumMap<KEY, Stats> map = new EnumMap<>(enumClass);
for (KEY value : values) {
- counters.put(value, new AtomicLong(0));
- elapsedTimes.put(value, new AtomicLong(0));
+ map.put(value, new Stats());
}
+ this.stats = Collections.unmodifiableMap(map);
}
Review Comment:
- Pass the class instead of values.
- Change the constructor to private.
```java
public static <KEY extends Enum<KEY>> ProtocolMessageMetrics<KEY>
create(String name,
String description, Class<KEY> enumClass) {
return new ProtocolMessageMetrics<>(name, description, enumClass);
}
private ProtocolMessageMetrics(String name, String description, Class<KEY>
enumClass) {
this.name = name;
this.description = description;
final EnumMap<KEY, Stats> map = new EnumMap<>(enumClass);
for (KEY value : enumClass.getEnumConstants()) {
map.put(value, new Stats());
}
this.stats = Collections.unmodifiableMap(map);
}
```
--
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]
