mimaison commented on code in PR #19068:
URL: https://github.com/apache/kafka/pull/19068#discussion_r2027393106
##########
clients/clients-integration-tests/src/test/java/org/apache/kafka/server/quota/CustomQuotaCallbackTest.java:
##########
@@ -69,10 +70,37 @@ public void testCustomQuotaCallbackWithControllerServer()
throws InterruptedExce
&&
CustomQuotaCallback.COUNTERS.values().stream().allMatch(counter ->
counter.get() > 0),
"The CustomQuotaCallback not triggered in all controllers.
"
);
-
+
+ }
+ }
+
+ @ClusterTest(
+ types = {Type.CO_KRAFT, Type.KRAFT},
+ serverProperties = {
+ @ClusterConfigProperty(key =
QuotaConfig.CLIENT_QUOTA_CALLBACK_CLASS_CONFIG, value =
"org.apache.kafka.server.quota.CustomQuotaCallbackTest$MonitorableCustomQuotaCallback"),
}
+ )
+ public void
testMonitorableCustomQuotaCallbackWithCombinedMode(ClusterInstance cluster) {
+ assertEquals(2, MonitorableCustomQuotaCallback.metricNames.size());
+ MetricName metricName1 =
MonitorableCustomQuotaCallback.metricNames.get(0);
+ MetricName metricName2 =
MonitorableCustomQuotaCallback.metricNames.get(1);
+ if
(metricName1.tags().get("role").equals(ProcessRole.ControllerRole.toString())) {
Review Comment:
If you use the actual value to predict the expected value, in the end you're
not testing anything! Instead of that you can use `clusterInstance.type()` to
identify the mode the test is using.
##########
clients/clients-integration-tests/src/test/java/org/apache/kafka/server/quota/CustomQuotaCallbackTest.java:
##########
@@ -69,10 +70,37 @@ public void testCustomQuotaCallbackWithControllerServer()
throws InterruptedExce
&&
CustomQuotaCallback.COUNTERS.values().stream().allMatch(counter ->
counter.get() > 0),
"The CustomQuotaCallback not triggered in all controllers.
"
);
-
+
+ }
+ }
+
+ @ClusterTest(
+ types = {Type.CO_KRAFT, Type.KRAFT},
+ serverProperties = {
+ @ClusterConfigProperty(key =
QuotaConfig.CLIENT_QUOTA_CALLBACK_CLASS_CONFIG, value =
"org.apache.kafka.server.quota.CustomQuotaCallbackTest$MonitorableCustomQuotaCallback"),
}
+ )
+ public void
testMonitorableCustomQuotaCallbackWithCombinedMode(ClusterInstance cluster) {
+ assertEquals(2, MonitorableCustomQuotaCallback.metricNames.size());
+ MetricName metricName1 =
MonitorableCustomQuotaCallback.metricNames.get(0);
Review Comment:
Instead of using static fields, I think it's better to retrieve the metrics
from the brokers and controllers like I do in my test.
##########
clients/src/main/java/org/apache/kafka/server/quota/ClientQuotaCallback.java:
##########
@@ -24,6 +24,13 @@
/**
* Quota callback interface for brokers and controllers that enables
customization of client quota computation.
+ * Implement {@link org.apache.kafka.common.metrics.Monitorable} to enable the
callback to register metrics.
+ * The following tags are automatically added to all metrics registered:
+ * <ul>
+ * <li><code>config</code> set to
<code>clientQuotaCallback.class</code></li>
Review Comment:
The config is named `client.quota.callback.class`
##########
clients/clients-integration-tests/src/test/java/org/apache/kafka/server/quota/CustomQuotaCallbackTest.java:
##########
@@ -121,4 +149,23 @@ public void configure(Map<String, ?> configs) {
}
}
+
+ public static class MonitorableCustomQuotaCallback extends
CustomQuotaCallback implements Monitorable {
+
Review Comment:
nit: extra line
--
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
