[GitHub] [kafka] niket-goel commented on a diff in pull request #12448: KAFKA-14114: Adding Metadata Log Processing Error Related Metrics

[GitBox <gi...@apache.org>]

niket-goel commented on code in PR #12448:
URL: https://github.com/apache/kafka/pull/12448#discussion_r931715659


##########
core/src/main/scala/kafka/server/metadata/BrokerMetadataListener.scala:
##########
@@ -169,6 +174,10 @@ class BrokerMetadataListener(
         _delta.finishSnapshot()
         info(s"Loaded snapshot ${reader.snapshotId().offset}-${reader.snapshotId().epoch}: " +
           s"$loadResults")
+      } catch {
+        case e: Throwable =>
+          brokerMetrics.listenerBatchLoadErrorCount.getAndIncrement()
+          log.error(e.toString)

Review Comment:
   ditto



##########
metadata/src/test/java/org/apache/kafka/controller/QuorumControllerMetricsTest.java:
##########
@@ -151,10 +154,18 @@ private static MetricName metricName(String type, String name) {
     }
 
     private static void assertMetricsCreated(MetricsRegistry registry, Set<String> expectedMetricNames, String expectedType) {
+        assertEquals(registry.allMetrics().keySet().stream()

Review Comment:
   This test was not testing if any metrics other than the expected existed.



##########
core/src/main/scala/kafka/server/metadata/BrokerMetadataListener.scala:
##########
@@ -114,6 +114,11 @@ class BrokerMetadataListener(
           debug(s"Loaded new commits: $loadResults")
         }
         loadResults
+      } catch {
+        case e: Throwable =>
+          brokerMetrics.listenerBatchLoadErrorCount.getAndIncrement()
+          log.error(e.toString)
+          BatchLoadResults(0, 0, 0, 0)

Review Comment:
   This is the most sketchy part of the change. I was thinking of what a good way to handle errors in this should be. Is there something that would need to be undone in this scenario?



##########
metadata/src/main/java/org/apache/kafka/controller/QuorumControllerMetrics.java:
##########
@@ -276,12 +297,15 @@ public long lastAppliedRecordTimestamp() {
     public void close() {
         Arrays.asList(
             ACTIVE_CONTROLLER_COUNT,
+            FENCED_BROKER_COUNT,

Review Comment:
   Fixing the corresponding test surfaced the fact that we were not removing these metrics from the registry. Not sure if that was on purpose.



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org