C0urante commented on a change in pull request #10910:
URL: https://github.com/apache/kafka/pull/10910#discussion_r758864544
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/ErrorHandlingMetrics.java
##########
@@ -23,16 +23,20 @@
import org.apache.kafka.connect.runtime.ConnectMetrics;
import org.apache.kafka.connect.runtime.ConnectMetricsRegistry;
import org.apache.kafka.connect.util.ConnectorTaskId;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
/**
* Contains various sensors used for monitoring errors.
*/
-public class ErrorHandlingMetrics {
+public class ErrorHandlingMetrics {
Review comment:
Can this be reverted? The existing formatting appears to be correct.
```suggestion
public class ErrorHandlingMetrics {
```
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/Worker.java
##########
@@ -113,6 +113,7 @@
private final ConcurrentMap<String, WorkerConnector> connectors = new
ConcurrentHashMap<>();
private final ConcurrentMap<ConnectorTaskId, WorkerTask> tasks = new
ConcurrentHashMap<>();
+ private final ConcurrentMap<ConnectorTaskId, ErrorHandlingMetrics>
errorHandlingMetricsMap = new ConcurrentHashMap<>();
Review comment:
Rather than do this bookkeeping here, could we pass the
`ErrorHandlingMetrics` instance to the `WorkerTask` class in its constructor,
and then close it in `WorkerTask::removeMetrics`? It'd align nicely with the
existing contract for that method, which is that it will "Remove all metrics
published by this task."
##########
File path:
connect/runtime/src/main/java/org/apache/kafka/connect/runtime/errors/ErrorHandlingMetrics.java
##########
@@ -138,4 +142,12 @@ public void recordErrorTimestamp() {
public ConnectMetrics.MetricGroup metricGroup() {
return metricGroup;
}
+
+ /**
+ * Close the task Error metrics group when the task is closed
+ */
+ public void closeTaskErrorMetricGroup() {
Review comment:
Nit: whitespace
```suggestion
public void closeTaskErrorMetricGroup() {
```
Also, just curious, any reason we don't want to implement `Autocloseable`
and rename this method to `close`? It'd align nicely with the precedent set in
https://github.com/apache/kafka/pull/8442, for example, and would make this
class easer to use with
[Utils::closeQuietly](https://github.com/apache/kafka/blob/e8dcbb99bb3289193a9036599d87acd56e11499f/clients/src/main/java/org/apache/kafka/common/utils/Utils.java#L998-L1009)
if we wanted to go that route in the future.
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
