chia7712 commented on code in PR #17264:
URL: https://github.com/apache/kafka/pull/17264#discussion_r1976423672
##########
server/src/test/java/org/apache/kafka/server/metrics/ClientMetricsTestUtils.java:
##########
@@ -40,18 +40,18 @@ public class ClientMetricsTestUtils {
public static final String DEFAULT_METRICS =
"org.apache.kafka.client.producer.partition.queue.,org.apache.kafka.client.producer.partition.latency";
- public static final int DEFAULT_PUSH_INTERVAL_MS = 30 * 1000; // 30 seconds
- public static final List<String> DEFAULT_CLIENT_MATCH_PATTERNS = List.of(
+ public static final int DEFAULT_INTERVAL_MS = 30 * 1000; // 30 seconds
Review Comment:
Should we align the naming? `DEFAULT_INTERVAL_MS` -> `INTERVAL_MS_DEFAULT`
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java:
##########
@@ -110,6 +112,18 @@ public static ConfigDef configDef() {
return CONFIG;
}
+ public static Optional<Type> configType(String configName) {
+ return Optional.ofNullable(CONFIG.configKeys().get(configName)).map(c
-> c.type);
+ }
+
+ public static Map<String, Object> defaultConfigsMap() {
+ Map<String, Object> clientMetricsProps = new HashMap<>();
+ clientMetricsProps.put(METRICS_CONFIG, List.of());
Review Comment:
Should we create a `METRICS_CONFIG_DEFAULT` to replace `List.of()`?
##########
core/src/main/scala/kafka/server/ConfigHelper.scala:
##########
@@ -210,6 +201,28 @@ class ConfigHelper(metadataCache: MetadataCache, config:
KafkaConfig, configRepo
.setDocumentation(configDocumentation).setConfigType(dataType.id)
}
+ private def createClientMetricsConfigEntry(clientMetricsConfig:
ClientMetricsConfigs, clientMetricsProps: Properties, includeSynonyms: Boolean,
includeDocumentation: Boolean)
+ (name: String, value: Any):
DescribeConfigsResponseData.DescribeConfigsResourceResult = {
+ val configEntryType = ClientMetricsConfigs.configType(name).asScala
+ val valueAsString = ConfigDef.convertToString(value,
configEntryType.orNull)
+ val allSynonyms = {
+ if (!clientMetricsProps.containsKey(name)) {
+ Nil
Review Comment:
@AndrewJSchofield any feedback for this comment?
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java:
##########
@@ -110,6 +112,18 @@ public static ConfigDef configDef() {
return CONFIG;
}
+ public static Optional<Type> configType(String configName) {
+ return Optional.ofNullable(CONFIG.configKeys().get(configName)).map(c
-> c.type);
+ }
+
+ public static Map<String, Object> defaultConfigsMap() {
+ Map<String, Object> clientMetricsProps = new HashMap<>();
+ clientMetricsProps.put(METRICS_CONFIG, List.of());
+ clientMetricsProps.put(INTERVAL_MS_CONFIG, INTERVAL_MS_DEFAULT);
+ clientMetricsProps.put(MATCH_CONFIG, List.of());
Review Comment:
ditto
##########
server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java:
##########
@@ -98,9 +100,9 @@ public class ClientMetricsConfigs extends AbstractConfig {
));
private static final ConfigDef CONFIG = new ConfigDef()
- .define(SUBSCRIPTION_METRICS, Type.LIST, List.of(), Importance.MEDIUM,
"Subscription metrics list")
- .define(PUSH_INTERVAL_MS, Type.INT, DEFAULT_INTERVAL_MS,
Importance.MEDIUM, "Push interval in milliseconds")
- .define(CLIENT_MATCH_PATTERN, Type.LIST, List.of(), Importance.MEDIUM,
"Client match pattern list");
+ .define(METRICS_CONFIG, Type.LIST, List.of(), Importance.MEDIUM,
"Telemetry metric name prefix list")
+ .define(INTERVAL_MS_CONFIG, Type.INT, INTERVAL_MS_DEFAULT,
Importance.MEDIUM, "Metrics push interval in milliseconds")
+ .define(MATCH_CONFIG, Type.LIST, List.of(), Importance.MEDIUM, "Client
match criteria");
public ClientMetricsConfigs(Properties props) {
super(CONFIG, props);
Review Comment:
This is unrelated to this PR, but should we set `doLog=false`?
--
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
