You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "apoorvmittal10 (via GitHub)" <gi...@apache.org> on 2023/10/25 10:36:44 UTC
[PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
apoorvmittal10 opened a new pull request, #14632:
URL: https://github.com/apache/kafka/pull/14632
The PR adds support of alter/describe configs for client-metrics as defined in [KIP-714](https://cwiki.apache.org/confluence/display/KAFKA/KIP-714%3A+Client+metrics+and+observability#KIP714:Clientmetricsandobservability-kafka-configs.sh)
Below are the results of the commands:
#### Help section adds details for `client-metrics`:
```
./bin/kafka-configs.sh --help
This tool helps to manipulate and describe entity config for a topic, client, user, broker, ip or client-metrics
.
.
.
--add-config <String> Key Value pairs of configs to add.
Square brackets can be used to group
values which contain commas: 'k1=v1,
k2=[v1,v2,v2],k3=v3'. The following
is a list of valid configurations:
For entity-type 'topics':
.
.
.
controller_mutation_rate
producer_byte_rate
request_percentage
For entity-type 'clients':
consumer_byte_rate
controller_mutation_rate
producer_byte_rate
request_percentage
For entity-type 'ips':
connection_creation_rate
For entity-type 'client-metrics':
interval.ms
match
metrics
Entity types 'users' and 'clients' may
be specified together to update
config for clients of a specific
user.
--entity-type <String> Type of entity
(topics/clients/users/brokers/broker-
loggers/ips/client-metrics)
--entity-name <String> Name of entity (topic name/client
id/user principal name/broker
id/ip/client metrics subscription
name)
```
#### Incorrect entity type:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type client-metrics1 --describe --entity-name METRICSUB
Invalid entity type client-metrics1, the entity type must be one of topics, clients, users, brokers, ips, client-metrics, broker-loggers with a --bootstrap-server or --bootstrap-controller argument
```
#### Describe wihout entity name:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type client-metrics --describe
an entity name must be specified with --describe of client-metrics
```
#### Describe with blank entity name:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type client-metrics --describe --entity-name ""
an entity name must be specified with --describe of client-metrics
```
#### Invalid entity name. Omitted to throw exception as the describe response is further needed in alter to construct if the new subscription to be added or altered.
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type client-metrics --describe --entity-name "random"
Dynamic configs for client-metric random are:
```
#### Successful alter of `client-metrics`:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --alter --entity-type client-metrics --entity-name METRICSUB --add-config "metrics=org.apache.kafka.consumer.,interval.ms=60000,match=[client_software_name=kafka.python,client_software_version=1\.2\..*]"
Completed updating config for client-metric METRICSUB.
```
#### Successful describe of `client-metrics`:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type client-metrics --describe --entity-name METRICSUB
Dynamic configs for client-metric METRICSUB are:
interval.ms=60000 sensitive=false synonyms={}
match=client_software_name=kafka.python,client_software_version=1\.2\..* sensitive=false synonyms={}
metrics=org.apache.kafka.consumer. sensitive=false synonyms={}
```
### With Zookeper:
#### Without making change in `ZKConfigRepository`, throws UnknownServerException:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type client-metrics --describe --entity-name "random"
Dynamic configs for client-metric random are:
Error while executing config command with args '--bootstrap-server localhost:9092 --entity-type client-metrics --describe --entity-name random'
java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.UnknownServerException: The server experienced an unexpected error when processing the request.
at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:180)
at kafka.admin.ConfigCommand$.getResourceConfig(ConfigCommand.scala:614)
at kafka.admin.ConfigCommand$.$anonfun$describeResourceConfig$5(ConfigCommand.scala:572)
at kafka.admin.ConfigCommand$.$anonfun$describeResourceConfig$5$adapted(ConfigCommand.scala:564)
at scala.collection.immutable.List.foreach(List.scala:333)
at kafka.admin.ConfigCommand$.describeResourceConfig(ConfigCommand.scala:564)
at kafka.admin.ConfigCommand$.describeConfig(ConfigCommand.scala:542)
at kafka.admin.ConfigCommand$.processCommand(ConfigCommand.scala:343)
at kafka.admin.ConfigCommand$.main(ConfigCommand.scala:97)
at kafka.admin.ConfigCommand.main(ConfigCommand.scala)
Caused by: org.apache.kafka.common.errors.UnknownServerException: The server experienced an unexpected error when processing the request.
```
#### After the change in `ZKConfigRepository`:
#### Describe:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --entity-type client-metrics --describe --entity-name "random"
Dynamic configs for client-metric random are:
Error while executing config command with args '--bootstrap-server localhost:9092 --entity-type client-metrics --describe --entity-name random'
java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.InvalidRequestException: Config type client-metrics is only supported on KRaft clusters
at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:180)
at kafka.admin.ConfigCommand$.getResourceConfig(ConfigCommand.scala:614)
at kafka.admin.ConfigCommand$.$anonfun$describeResourceConfig$5(ConfigCommand.scala:572)
at kafka.admin.ConfigCommand$.$anonfun$describeResourceConfig$5$adapted(ConfigCommand.scala:564)
at scala.collection.immutable.List.foreach(List.scala:333)
at kafka.admin.ConfigCommand$.describeResourceConfig(ConfigCommand.scala:564)
at kafka.admin.ConfigCommand$.describeConfig(ConfigCommand.scala:542)
at kafka.admin.ConfigCommand$.processCommand(ConfigCommand.scala:343)
at kafka.admin.ConfigCommand$.main(ConfigCommand.scala:97)
at kafka.admin.ConfigCommand.main(ConfigCommand.scala)
Caused by: org.apache.kafka.common.errors.InvalidRequestException: Config type client-metrics is only supported on KRaft clusters
```
#### Alter:
```
./bin/kafka-configs.sh --bootstrap-server localhost:9092 --alter --entity-type client-metrics --entity-name METRICSUB2 --add-config "interval.ms=1000"
Error while executing config command with args '--bootstrap-server localhost:9092 --alter --entity-type client-metrics --entity-name METRICSUB2 --add-config interval.ms=1000'
java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.InvalidRequestException: Config type client-metrics is only supported on KRaft clusters
at java.base/java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:396)
at java.base/java.util.concurrent.CompletableFuture.get(CompletableFuture.java:2096)
at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:180)
at kafka.admin.ConfigCommand$.getResourceConfig(ConfigCommand.scala:610)
at kafka.admin.ConfigCommand$.alterConfig(ConfigCommand.scala:450)
at kafka.admin.ConfigCommand$.processCommand(ConfigCommand.scala:341)
at kafka.admin.ConfigCommand$.main(ConfigCommand.scala:97)
at kafka.admin.ConfigCommand.main(ConfigCommand.scala)
Caused by: org.apache.kafka.common.errors.InvalidRequestException: Config type client-metrics is only supported on KRaft clusters
```
#### With zookeeper as arg:
```
./bin/kafka-configs.sh -zookeeper localhost:9071 --describe --entity-type client-metrics --entity-name METRICSUB2
Invalid entity type client-metrics, the entity type must be one of users, brokers with a --zookeeper argument
```
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1806853354
@apoorvmittal10 : Thanks for rerunning the tests. Are the 34 test failures related to this PR?
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 closed pull request #14632: KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714)
URL: https://github.com/apache/kafka/pull/14632
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1807647129
Triggered the build again.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1807644234
Build passes on all platforms but there are some test failures which seems not related to the code changes in the PR.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1802514338
> @apoorvmittal10 : Thanks for the PR. Left a few comments.
Thanks for the feedback @junrao. I have addressed the comments.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1809524348
There are different tests failing from previous runs and none related to the PR.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1387417505
##########
core/src/main/scala/kafka/server/DynamicConfig.scala:
##########
@@ -111,6 +112,16 @@ object DynamicConfig {
}
}
+ object ClientMetrics {
+ private val clientConfigs = kafka.metrics.ClientMetricsConfigs.configDef()
+
+ def configKeys: util.Map[String, ConfigDef.ConfigKey] = clientConfigs.configKeys
+
+ def names: util.Set[String] = clientConfigs.names
+
+ def validate(props: Properties) = DynamicConfig.validate(clientConfigs, props, customPropsAllowed = false)
Review Comment:
My bad, should have not included in the PR itself and then misread the comment. I have removed it, apologies.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1371639259
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -576,6 +594,8 @@ object ConfigCommand extends Logging {
if (entityName.nonEmpty)
validateBrokerId()
(ConfigResource.Type.BROKER_LOGGER, None)
+ case ConfigType.ClientMetrics =>
+ (ConfigResource.Type.CLIENT_METRICS, Some(ConfigEntry.ConfigSource.DYNAMIC_CLIENT_METRICS_CONFIG))
Review Comment:
Yeah, I need to support describe all which shall require either adding an API in admin client or need to work something with in-memory cache of subscriptions. I ll figure that out as we proceed with other code changes.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1825923669
New failing - 12
Build / JDK 11 and Scala 2.13 / testAlterSinkConnectorOffsets – org.apache.kafka.connect.integration.OffsetsApiIntegrationTest
12s
Build / JDK 11 and Scala 2.13 / testResetSinkConnectorOffsetsOverriddenConsumerGroupId – org.apache.kafka.connect.integration.OffsetsApiIntegrationTest
51s
Build / JDK 11 and Scala 2.13 / testResetSinkConnectorOffsetsZombieSinkTasks – org.apache.kafka.connect.integration.OffsetsApiIntegrationTest
58s
Build / JDK 21 and Scala 2.13 / testReplicateSourceDefault() – org.apache.kafka.connect.mirror.integration.IdentityReplicationIntegrationTest
1m 50s
Build / JDK 21 and Scala 2.13 / testSyncTopicConfigs() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationTransactionsTest
1m 42s
Build / JDK 21 and Scala 2.13 / shouldAddAndRemoveNamedTopologiesBeforeStartingAndRouteQueriesToCorrectTopology() – org.apache.kafka.streams.integration.NamedTopologyIntegrationTest
1m 9s
Build / JDK 21 and Scala 2.13 / testTaskRequestWithOldStartMsGetsUpdated() – org.apache.kafka.trogdor.coordinator.CoordinatorTest
2m 0s
Build / JDK 17 and Scala 2.13 / testTaskRequestWithOldStartMsGetsUpdated() – org.apache.kafka.trogdor.coordinator.CoordinatorTest
2m 0s
Build / JDK 8 and Scala 2.12 / testConsumptionWithBrokerFailures() – kafka.api.ConsumerBounceTest
54s
Build / JDK 8 and Scala 2.12 / testAlwaysSendsAccumulatedOfflineDirs() – kafka.server.BrokerLifecycleManagerTest
<1s
Build / JDK 8 and Scala 2.12 / testInternalTopicExists() – org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManagerTest
12s
Build / JDK 8 and Scala 2.12 / testListTopicsWithExcludeInternal(String).quorum=zk – org.apache.kafka.tools.TopicCommandIntegrationTest: https://issues.apache.org/jira/browse/KAFKA-15140
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1813025066
> @apoorvmittal10 : Thanks for looking into the test failures. There is an ongoing discussion on requiring a green build before merging the PR. I will need to wait for the result of that discussion before merging the PR.
Thanks @junrao I followed the thread where discussion is going on for green builds, started by @dajac, though I am not so old with AK process but if we are targeting no test failures in PRs for Kafka then shouldn't we be aggressive in fixing those else it will delay all deliverables for 3.7. I ll wait for the decision or what's the way forward. Definitely flaky tests is a big problem with AK right now.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1820775191
@junrao @mjsax There are still some flaky tests which fails, Can I do anything to get the build green?
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 closed pull request #14632: KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714)
URL: https://github.com/apache/kafka/pull/14632
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1806869168
> @apoorvmittal10 : Thanks for rerunning the tests. Are the 34 test failures related to this PR?
@junrao I looked at the failing tests and none of them seems related to the PR changes. I saw the comment and mail by @dajac and completely agree with the pain of getting the build without flaky tests for a while.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1371581178
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -445,6 +446,21 @@ object ConfigCommand extends Logging {
if (unknownConfigs.nonEmpty)
throw new IllegalArgumentException(s"Only connection quota configs can be added for '${ConfigType.Ip}' using --bootstrap-server. Unexpected config names: ${unknownConfigs.mkString(",")}")
alterQuotaConfigs(adminClient, entityTypes, entityNames, configsToBeAddedMap, configsToBeDeleted)
+ case ConfigType.ClientMetrics =>
+ val oldConfig = getResourceConfig(adminClient, entityTypeHead, entityNameHead, includeSynonyms = false, describeAll = false)
+ .map { entry => (entry.name, entry) }.toMap
+
+ // fail the command if any of the configs to be deleted does not exist
+ val invalidConfigs = configsToBeDeleted.filterNot(oldConfig.contains)
+ if (invalidConfigs.nonEmpty)
+ throw new InvalidConfigurationException(s"Invalid config(s): ${invalidConfigs.mkString(",")}")
Review Comment:
Actually. I see that you're just copying the rest of the class. Don't worry.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1818256808
10 Failing flaky tests in current run, some have existing jira (mostly open):
Build / JDK 11 and Scala 2.13 / testRackAwareRangeAssignor(String).quorum=kraft – integration.kafka.server.FetchFromFollowerIntegrationTest
5s - https://issues.apache.org/jira/browse/KAFKA-15020
Build / JDK 11 and Scala 2.13 / testFailureToFenceEpoch(String).quorum=kraft – org.apache.kafka.tiered.storage.integration.TransactionsWithTieredStoreTest
38s - https://issues.apache.org/jira/browse/KAFKA-14989
Build / JDK 8 and Scala 2.12 / testDynamicProducerIdExpirationMs(String).quorum=kraft – kafka.api.ProducerIdExpirationTest
32s
Build / JDK 8 and Scala 2.12 / testThrottledProducerConsumer(String).quorum=zk – kafka.api.UserClientIdQuotaTest
41s
Build / JDK 8 and Scala 2.12 / testThrottledProducerConsumer(String).quorum=kraft – kafka.api.UserClientIdQuotaTest
43s
Build / JDK 8 and Scala 2.12 / testQuotaOverrideDelete(String).quorum=zk – kafka.api.UserClientIdQuotaTest
1m 6s
Build / JDK 8 and Scala 2.12 / testAssignmentAggregation() – kafka.server.AssignmentsManagerTest
<1s
Build / JDK 8 and Scala 2.12 / testAssignmentAggregation() – kafka.server.AssignmentsManagerTest
2s
Build / JDK 8 and Scala 2.12 / shouldMigratePersistentKeyValueStoreToTimestampedKeyValueStoreUsingPapi – org.apache.kafka.streams.integration.StoreUpgradeIntegrationTest
1m 7s - https://issues.apache.org/jira/browse/KAFKA-10151
Build / JDK 8 and Scala 2.12 / [6] Type=Raft-Isolated, Name=testDescribeQuorumReplicationSuccessful, MetadataVersion=3.7-IV1, Security=PLAINTEXT – org.apache.kafka.tools.MetadataQuorumCommandTest
1m 7s - https://issues.apache.org/jira/browse/KAFKA-15104
Build / JDK 17 and Scala 2.13 / testTaskRequestWithOldStartMsGetsUpdated() – org.apache.kafka.trogdor.coordinator.CoordinatorTest
2m 0s - https://issues.apache.org/jira/browse/KAFKA-15760
Build / JDK 21 and Scala 2.13 / shouldThrowIllegalArgumentExceptionWhenCustomPartitionerReturnsMultiplePartitions() – org.apache.kafka.streams.integration.KTableKTableForeignKeyInnerJoinCustomPartitionerIntegrationTest
1m 5s - https://issues.apache.org/jira/browse/KAFKA-14454
Build / JDK 21 and Scala 2.13 / [1] Type=Raft-Combined, Name=testDescribeQuorumReplicationSuccessful, MetadataVersion=3.7-IV1, Security=PLAINTEXT – org.apache.kafka.tools.MetadataQuorumCommandTest
1m 7s - https://issues.apache.org/jira/browse/KAFKA-15104
Build / JDK 21 and Scala 2.13 / [5] Type=Raft-Combined, Name=testDescribeQuorumReplicationSuccessful, MetadataVersion=3.7-IV1, Security=PLAINTEXT – org.apache.kafka.tools.MetadataQuorumCommandTest
https://issues.apache.org/jira/browse/KAFKA-15104
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1385792007
##########
core/src/main/scala/kafka/server/DynamicConfig.scala:
##########
@@ -111,6 +112,16 @@ object DynamicConfig {
}
}
+ object ClientMetrics {
+ private val clientConfigs = kafka.metrics.ClientMetricsConfigs.configDef()
+
+ def configKeys: util.Map[String, ConfigDef.ConfigKey] = clientConfigs.configKeys
+
+ def names: util.Set[String] = clientConfigs.names
+
+ def validate(props: Properties) = DynamicConfig.validate(clientConfigs, props, customPropsAllowed = false)
Review Comment:
This is for ZK-based controller. Since ClientMetrics is not supported there, do we need this? Ditto for `configKeys`.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -536,6 +552,8 @@ object ConfigCommand extends Logging {
adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
case ConfigType.Broker | BrokerLoggerConfigType =>
adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName
+ case ConfigType.ClientMetrics =>
+ throw new InvalidRequestException("Client metrics entity-name is required")
Review Comment:
I guess this will be improved through https://cwiki.apache.org/confluence/display/KAFKA/KIP-1000%3A+List+Client+Metrics+Configuration+Resources ?
##########
core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala:
##########
@@ -1628,6 +1628,122 @@ class ConfigCommandTest extends Logging {
Seq("<default>/clients/client-3", sanitizedPrincipal + "/clients/client-2"))
}
+ @Test
+ def shouldAlterClientMetricsConfig(): Unit = {
+ val node = new Node(1, "localhost", 9092)
+ verifyAlterClientMetricsConfig(node, "1", List("--entity-name", "1"))
+ }
+
+ def verifyAlterClientMetricsConfig(node: Node, resourceName: String, resourceOpts: List[String]): Unit = {
Review Comment:
Could this be private?
##########
core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala:
##########
@@ -1628,6 +1628,122 @@ class ConfigCommandTest extends Logging {
Seq("<default>/clients/client-3", sanitizedPrincipal + "/clients/client-2"))
}
+ @Test
+ def shouldAlterClientMetricsConfig(): Unit = {
+ val node = new Node(1, "localhost", 9092)
+ verifyAlterClientMetricsConfig(node, "1", List("--entity-name", "1"))
+ }
+
+ def verifyAlterClientMetricsConfig(node: Node, resourceName: String, resourceOpts: List[String]): Unit = {
+ val optsList = List("--bootstrap-server", "localhost:9092",
+ "--entity-type", "client-metrics",
+ "--alter",
+ "--delete-config", "interval.ms",
+ "--add-config", "metrics=org.apache.kafka.consumer.," +
+ "match=[client_software_name=kafka.python,client_software_version=1\\.2\\..*]") ++ resourceOpts
+ val alterOpts = new ConfigCommandOptions(optsList.toArray)
+
+ val resource = new ConfigResource(ConfigResource.Type.CLIENT_METRICS, resourceName)
+ val configEntries = util.Collections.singletonList(new ConfigEntry("interval.ms", "1000",
+ ConfigEntry.ConfigSource.DYNAMIC_CLIENT_METRICS_CONFIG, false, false, Collections.emptyList,
+ ConfigEntry.ConfigType.UNKNOWN, null))
+ val future = new KafkaFutureImpl[util.Map[ConfigResource, Config]]
+ future.complete(util.Collections.singletonMap(resource, new Config(configEntries)))
+ val describeResult: DescribeConfigsResult = mock(classOf[DescribeConfigsResult])
+ when(describeResult.all()).thenReturn(future)
+
+ val alterFuture = new KafkaFutureImpl[Void]
+ alterFuture.complete(null)
+ val alterResult: AlterConfigsResult = mock(classOf[AlterConfigsResult])
+ when(alterResult.all()).thenReturn(alterFuture)
+
+ val mockAdminClient = new MockAdminClient(util.Collections.singletonList(node), node) {
+ override def describeConfigs(resources: util.Collection[ConfigResource], options: DescribeConfigsOptions): DescribeConfigsResult = {
+ assertFalse(options.includeSynonyms(), "Config synonyms requested unnecessarily")
+ assertEquals(1, resources.size)
+ val resource = resources.iterator.next
+ assertEquals(ConfigResource.Type.CLIENT_METRICS, resource.`type`)
+ assertEquals(resourceName, resource.name)
+ describeResult
+ }
+
+ override def incrementalAlterConfigs(configs: util.Map[ConfigResource, util.Collection[AlterConfigOp]], options: AlterConfigsOptions): AlterConfigsResult = {
+ assertEquals(1, configs.size)
+ val entry = configs.entrySet.iterator.next
+ val resource = entry.getKey
+ val alterConfigOps = entry.getValue
+ assertEquals(ConfigResource.Type.CLIENT_METRICS, resource.`type`)
+ assertEquals(3, alterConfigOps.size)
+
+ val expectedConfigOps = List(
+ new AlterConfigOp(new ConfigEntry("match", "client_software_name=kafka.python,client_software_version=1\\.2\\..*"), AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry("metrics", "org.apache.kafka.consumer."), AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry("interval.ms", ""), AlterConfigOp.OpType.DELETE)
+ )
+ assertEquals(expectedConfigOps, alterConfigOps.asScala.toList)
+ alterResult
+ }
+ }
+ ConfigCommand.alterConfig(mockAdminClient, alterOpts)
+ verify(describeResult).all()
Review Comment:
Do we need to verify `alterResult` too? Also, why do we need to call `all()`?
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 closed pull request #14632: KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714)
URL: https://github.com/apache/kafka/pull/14632
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1806857151
@junrao FYI - There is a couple of failed tests in trunk:
* https://github.com/apache/kafka/pull/14738
* https://github.com/apache/kafka/pull/14739
* https://github.com/apache/kafka/pull/14741
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1371529124
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -445,6 +446,21 @@ object ConfigCommand extends Logging {
if (unknownConfigs.nonEmpty)
throw new IllegalArgumentException(s"Only connection quota configs can be added for '${ConfigType.Ip}' using --bootstrap-server. Unexpected config names: ${unknownConfigs.mkString(",")}")
alterQuotaConfigs(adminClient, entityTypes, entityNames, configsToBeAddedMap, configsToBeDeleted)
+ case ConfigType.ClientMetrics =>
+ val oldConfig = getResourceConfig(adminClient, entityTypeHead, entityNameHead, includeSynonyms = false, describeAll = false)
+ .map { entry => (entry.name, entry) }.toMap
+
+ // fail the command if any of the configs to be deleted does not exist
+ val invalidConfigs = configsToBeDeleted.filterNot(oldConfig.contains)
+ if (invalidConfigs.nonEmpty)
+ throw new InvalidConfigurationException(s"Invalid config(s): ${invalidConfigs.mkString(",")}")
Review Comment:
This error message could be clearer.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -536,6 +552,8 @@ object ConfigCommand extends Logging {
adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
case ConfigType.Broker | BrokerLoggerConfigType =>
adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName
+ case ConfigType.ClientMetrics =>
+ adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq
Review Comment:
I expect this needs to be `adminClient.describeConfigs`.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -576,6 +594,8 @@ object ConfigCommand extends Logging {
if (entityName.nonEmpty)
validateBrokerId()
(ConfigResource.Type.BROKER_LOGGER, None)
+ case ConfigType.ClientMetrics =>
+ (ConfigResource.Type.CLIENT_METRICS, Some(ConfigEntry.ConfigSource.DYNAMIC_CLIENT_METRICS_CONFIG))
Review Comment:
Probably ought to have a non-empty entity name, or describeAll.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1371642341
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -536,6 +552,8 @@ object ConfigCommand extends Logging {
adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
case ConfigType.Broker | BrokerLoggerConfigType =>
adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName
+ case ConfigType.ClientMetrics =>
+ adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq
Review Comment:
I missed this, actually this is the place where if entity name is null then utility fetched all by issuing admin client API. I have added an exception here which we shall figure out when supporting describe all.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1804996531
> @apoorvmittal10 : Thanks for the updated PR. LGTM
>
> JDK 17 and Scala 2.13 didn't finish. Could you trigger another test run? This can typically be done by closing the PR, waiting for 20 secs and reopening it.
Thanks @junrao, I have merged upstream/trunk branch to trigger the build for now.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1819538489
@apoorvmittal10 : Thanks for triaging the failed tests. There is still no green build though.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1821564843
@apoorvmittal10 :
1. Do you know why JDK 11 and Scala 2.13 didn't build?
2. For getting green build, it would be useful to help triage the new test failures. If we could identify the PR that introduced the failure, we could ping the author for a fix.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1830342699
@apoorvmittal10 : Thanks for triaging the failed tests. Since they are unrelated to this PR, will merge it.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "AndrewJSchofield (via GitHub)" <gi...@apache.org>.
AndrewJSchofield commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1390269923
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -791,10 +811,10 @@ object ConfigCommand extends Logging {
val describeOpt = parser.accepts("describe", "List configs for the given entity.")
val allOpt = parser.accepts("all", "List all configs for the given topic, broker, or broker-logger entity (includes static configuration when the entity type is brokers)")
- val entityType = parser.accepts("entity-type", "Type of entity (topics/clients/users/brokers/broker-loggers/ips)")
+ val entityType = parser.accepts("entity-type", "Type of entity (topics/clients/users/brokers/broker-loggers/ips/client-metrics)")
.withRequiredArg
.ofType(classOf[String])
- val entityName = parser.accepts("entity-name", "Name of entity (topic name/client id/user principal name/broker id/ip)")
+ val entityName = parser.accepts("entity-name", "Name of entity (topic name/client id/user principal name/broker id/ip/client metrics subscription name)")
Review Comment:
Strictly speaking, this is not the client metrics subscription name. It's the name of the client metrics config resource. That's quite a mouthful, so I'd put `/ip/client metrics`.
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -916,6 +925,34 @@ synchronized private Throwable handleIncrementalResourceAlteration(
topicMetadata.configs = newMap;
return null;
}
+ case CLIENT_METRICS: {
+ String subscriptionName = resource.name();
Review Comment:
Again, `resourceName` I think is better.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -536,6 +552,8 @@ object ConfigCommand extends Logging {
adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
case ConfigType.Broker | BrokerLoggerConfigType =>
adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName
+ case ConfigType.ClientMetrics =>
+ throw new InvalidRequestException("Client metrics entity-name is required")
Review Comment:
I've written `kafka-client-metrics.sh` also which will be in a new PR once this one is merged.
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -823,6 +825,13 @@ synchronized private Config getResourceDescription(ConfigResource resource) {
}
throw new UnknownTopicOrPartitionException("Resource " + resource + " not found.");
}
+ case CLIENT_METRICS: {
+ String subscriptionName = resource.name();
Review Comment:
I'd call it `resourceName`. It's not the subscription name - the subscription is the set of metrics which is sent by the broker to a specific client instance, and that's composed of the client metrics config resources which match the client.
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -536,6 +552,8 @@ object ConfigCommand extends Logging {
adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
case ConfigType.Broker | BrokerLoggerConfigType =>
adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName
+ case ConfigType.ClientMetrics =>
+ throw new InvalidRequestException("Client metrics entity-name is required")
Review Comment:
Precisely.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 closed pull request #14632: KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714)
URL: https://github.com/apache/kafka/pull/14632
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1387077385
##########
core/src/main/scala/kafka/server/DynamicConfig.scala:
##########
@@ -111,6 +112,16 @@ object DynamicConfig {
}
}
+ object ClientMetrics {
+ private val clientConfigs = kafka.metrics.ClientMetricsConfigs.configDef()
+
+ def configKeys: util.Map[String, ConfigDef.ConfigKey] = clientConfigs.configKeys
+
+ def names: util.Set[String] = clientConfigs.names
+
+ def validate(props: Properties) = DynamicConfig.validate(clientConfigs, props, customPropsAllowed = false)
Review Comment:
I might be wrong but isn't it required to list all ConfigCommandOptions i.e. below line added in ConfigCommand.scala to list options for client-metrics.
```
"For entity-type '" + ConfigType.ClientMetrics + "': " + DynamicConfig.ClientMetrics.names.asScala.toSeq.sorted.map("\t" + _).mkString(nl, nl, nl) +
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1387072536
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -536,6 +552,8 @@ object ConfigCommand extends Logging {
adminClient.listTopics(new ListTopicsOptions().listInternal(true)).names().get().asScala.toSeq
case ConfigType.Broker | BrokerLoggerConfigType =>
adminClient.describeCluster(new DescribeClusterOptions()).nodes().get().asScala.map(_.idString).toSeq :+ BrokerDefaultEntityName
+ case ConfigType.ClientMetrics =>
+ throw new InvalidRequestException("Client metrics entity-name is required")
Review Comment:
Yes, it will be improved in KIP-1000 where empty entity-name can be used to describe all.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1387099016
##########
core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala:
##########
@@ -1628,6 +1628,122 @@ class ConfigCommandTest extends Logging {
Seq("<default>/clients/client-3", sanitizedPrincipal + "/clients/client-2"))
}
+ @Test
+ def shouldAlterClientMetricsConfig(): Unit = {
+ val node = new Node(1, "localhost", 9092)
+ verifyAlterClientMetricsConfig(node, "1", List("--entity-name", "1"))
+ }
+
+ def verifyAlterClientMetricsConfig(node: Node, resourceName: String, resourceOpts: List[String]): Unit = {
Review Comment:
Thanks, done.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao merged PR #14632:
URL: https://github.com/apache/kafka/pull/14632
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1820231138
> Seems there is a compilation error on the last run:
>
> ```
>
> [Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14632/core/src/main/scala/kafka/server/DynamicConfig.scala:116:47: object ClientMetricsConfigs is not a member of package kafka.metrics
>
> [Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14632/core/src/main/scala/kafka/server/DynamicConfig.scala:116:17: private val clientConfigs in object ClientMetrics is never used
> ```
The change in dependent classes broke the PR build. I have resolved merge issue and triggered build.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 closed pull request #14632: KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714)
URL: https://github.com/apache/kafka/pull/14632
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1387265452
##########
core/src/main/scala/kafka/server/DynamicConfig.scala:
##########
@@ -111,6 +112,16 @@ object DynamicConfig {
}
}
+ object ClientMetrics {
+ private val clientConfigs = kafka.metrics.ClientMetricsConfigs.configDef()
+
+ def configKeys: util.Map[String, ConfigDef.ConfigKey] = clientConfigs.configKeys
+
+ def names: util.Set[String] = clientConfigs.names
+
+ def validate(props: Properties) = DynamicConfig.validate(clientConfigs, props, customPropsAllowed = false)
Review Comment:
The code you pasted only uses `ClientMetrics.names`. Are `ClientMetrics.validate` and `ClientMetrics.configKeys` ever used?
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1800093637
@junrao @hachikuji @AndrewJSchofield @mjsax Please if I can get the feedback on the PR.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1390331867
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -823,6 +825,13 @@ synchronized private Config getResourceDescription(ConfigResource resource) {
}
throw new UnknownTopicOrPartitionException("Resource " + resource + " not found.");
}
+ case CLIENT_METRICS: {
+ String subscriptionName = resource.name();
Review Comment:
Make sense, done.
##########
clients/src/test/java/org/apache/kafka/clients/admin/MockAdminClient.java:
##########
@@ -916,6 +925,34 @@ synchronized private Throwable handleIncrementalResourceAlteration(
topicMetadata.configs = newMap;
return null;
}
+ case CLIENT_METRICS: {
+ String subscriptionName = resource.name();
Review Comment:
Done.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1807645072
Failing tests:
```
New failing - 15
Build / JDK 11 and Scala 2.13 / idleExpiryWithBufferedReceives() – kafka.network.SocketServerTest
18s
Build / JDK 17 and Scala 2.13 / testAlterPartitionVersion2KeepWorkingWhenControllerDowngradeToPre28IBP() – kafka.controller.ControllerIntegrationTest
1s
Build / JDK 17 and Scala 2.13 / testClose() – kafka.log.remote.RemoteIndexCacheTest
<1s
Build / JDK 17 and Scala 2.13 / testFenceMultipleBrokers() – org.apache.kafka.controller.QuorumControllerTest
52s
Build / JDK 17 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
15s
Build / JDK 17 and Scala 2.13 / testHighAvailabilityTaskAssignorLargePartitionCount – org.apache.kafka.streams.processor.internals.StreamsAssignmentScaleTest
1m 11s
Build / JDK 17 and Scala 2.13 / testTaskRequestWithOldStartMsGetsUpdated() – org.apache.kafka.trogdor.coordinator.CoordinatorTest
2m 0s
Build / JDK 21 and Scala 2.13 / testWithGroupId() – kafka.api.TransactionsBounceTest
20s
Build / JDK 21 and Scala 2.13 / testRegexMatchesTopicsAWhenDeleted() – org.apache.kafka.streams.integration.RegexSourceIntegrationTest
15s
Build / JDK 8 and Scala 2.12 / testOffsetTranslationBehindReplicationFlow() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationSSLTest
1m 7s
Build / JDK 8 and Scala 2.12 / testProduceConsumeTopicAutoCreateTopicCreateAcl(String).quorum=zk – kafka.api.GroupEndToEndAuthorizationTest
6s
Build / JDK 8 and Scala 2.12 / testFailureToFenceEpoch(String).quorum=kraft – kafka.api.TransactionsTest
39s
Build / JDK 8 and Scala 2.12 / [1] Type=ZK, MetadataVersion=3.4-IV0, Security=PLAINTEXT – kafka.zk.ZkMigrationIntegrationTest
11s
Build / JDK 8 and Scala 2.12 / [2] Type=ZK, MetadataVersion=3.5-IV2, Security=PLAINTEXT – kafka.zk.ZkMigrationIntegrationTest
17s
Build / JDK 8 and Scala 2.12 / [3] Type=ZK, MetadataVersion=3.6-IV2, Security=PLAINTEXT – kafka.zk.ZkMigrationIntegrationTest
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1387101615
##########
core/src/test/scala/unit/kafka/admin/ConfigCommandTest.scala:
##########
@@ -1628,6 +1628,122 @@ class ConfigCommandTest extends Logging {
Seq("<default>/clients/client-3", sanitizedPrincipal + "/clients/client-2"))
}
+ @Test
+ def shouldAlterClientMetricsConfig(): Unit = {
+ val node = new Node(1, "localhost", 9092)
+ verifyAlterClientMetricsConfig(node, "1", List("--entity-name", "1"))
+ }
+
+ def verifyAlterClientMetricsConfig(node: Node, resourceName: String, resourceOpts: List[String]): Unit = {
+ val optsList = List("--bootstrap-server", "localhost:9092",
+ "--entity-type", "client-metrics",
+ "--alter",
+ "--delete-config", "interval.ms",
+ "--add-config", "metrics=org.apache.kafka.consumer.," +
+ "match=[client_software_name=kafka.python,client_software_version=1\\.2\\..*]") ++ resourceOpts
+ val alterOpts = new ConfigCommandOptions(optsList.toArray)
+
+ val resource = new ConfigResource(ConfigResource.Type.CLIENT_METRICS, resourceName)
+ val configEntries = util.Collections.singletonList(new ConfigEntry("interval.ms", "1000",
+ ConfigEntry.ConfigSource.DYNAMIC_CLIENT_METRICS_CONFIG, false, false, Collections.emptyList,
+ ConfigEntry.ConfigType.UNKNOWN, null))
+ val future = new KafkaFutureImpl[util.Map[ConfigResource, Config]]
+ future.complete(util.Collections.singletonMap(resource, new Config(configEntries)))
+ val describeResult: DescribeConfigsResult = mock(classOf[DescribeConfigsResult])
+ when(describeResult.all()).thenReturn(future)
+
+ val alterFuture = new KafkaFutureImpl[Void]
+ alterFuture.complete(null)
+ val alterResult: AlterConfigsResult = mock(classOf[AlterConfigsResult])
+ when(alterResult.all()).thenReturn(alterFuture)
+
+ val mockAdminClient = new MockAdminClient(util.Collections.singletonList(node), node) {
+ override def describeConfigs(resources: util.Collection[ConfigResource], options: DescribeConfigsOptions): DescribeConfigsResult = {
+ assertFalse(options.includeSynonyms(), "Config synonyms requested unnecessarily")
+ assertEquals(1, resources.size)
+ val resource = resources.iterator.next
+ assertEquals(ConfigResource.Type.CLIENT_METRICS, resource.`type`)
+ assertEquals(resourceName, resource.name)
+ describeResult
+ }
+
+ override def incrementalAlterConfigs(configs: util.Map[ConfigResource, util.Collection[AlterConfigOp]], options: AlterConfigsOptions): AlterConfigsResult = {
+ assertEquals(1, configs.size)
+ val entry = configs.entrySet.iterator.next
+ val resource = entry.getKey
+ val alterConfigOps = entry.getValue
+ assertEquals(ConfigResource.Type.CLIENT_METRICS, resource.`type`)
+ assertEquals(3, alterConfigOps.size)
+
+ val expectedConfigOps = List(
+ new AlterConfigOp(new ConfigEntry("match", "client_software_name=kafka.python,client_software_version=1\\.2\\..*"), AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry("metrics", "org.apache.kafka.consumer."), AlterConfigOp.OpType.SET),
+ new AlterConfigOp(new ConfigEntry("interval.ms", ""), AlterConfigOp.OpType.DELETE)
+ )
+ assertEquals(expectedConfigOps, alterConfigOps.asScala.toList)
+ alterResult
+ }
+ }
+ ConfigCommand.alterConfig(mockAdminClient, alterOpts)
+ verify(describeResult).all()
Review Comment:
I have added alterResult as well, thanks. `all()` is need as it ensures the verification of returned `future` is executed.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1390331820
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -791,10 +811,10 @@ object ConfigCommand extends Logging {
val describeOpt = parser.accepts("describe", "List configs for the given entity.")
val allOpt = parser.accepts("all", "List all configs for the given topic, broker, or broker-logger entity (includes static configuration when the entity type is brokers)")
- val entityType = parser.accepts("entity-type", "Type of entity (topics/clients/users/brokers/broker-loggers/ips)")
+ val entityType = parser.accepts("entity-type", "Type of entity (topics/clients/users/brokers/broker-loggers/ips/client-metrics)")
.withRequiredArg
.ofType(classOf[String])
- val entityName = parser.accepts("entity-name", "Name of entity (topic name/client id/user principal name/broker id/ip)")
+ val entityName = parser.accepts("entity-name", "Name of entity (topic name/client id/user principal name/broker id/ip/client metrics subscription name)")
Review Comment:
Make sense, removed subscription name.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1810855040
@apoorvmittal10 : Thanks for looking into the test failures. There is an ongoing discussion on requiring a green build before merging the PR. I will need to wait for the result of that discussion before merging the PR.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "mjsax (via GitHub)" <gi...@apache.org>.
mjsax commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1820056764
Seems there is a compilation error on the last run:
```ask :core:compileScala
[Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14632/core/src/main/scala/kafka/server/DynamicConfig.scala:116:47: object ClientMetricsConfigs is not a member of package kafka.metrics
[Error] /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-14632/core/src/main/scala/kafka/server/DynamicConfig.scala:116:17: private val clientConfigs in object ClientMetrics is never used
```
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1822777968
> @apoorvmittal10 :
>
> 1. Do you know why JDK 11 and Scala 2.13 didn't build?
> 2. For getting green build, it would be useful to help triage the new test failures. If we could identify the PR that introduced the failure, we could ping the author for a fix.
I ll try to find out some details regarding failing tests. Build for JDK11 sems stuck in publishing test result where the build went for more than 7 hours.
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on PR #14632:
URL: https://github.com/apache/kafka/pull/14632#issuecomment-1779271622
Build depends on PR - https://github.com/apache/kafka/pull/14621
--
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
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
Posted by "apoorvmittal10 (via GitHub)" <gi...@apache.org>.
apoorvmittal10 commented on code in PR #14632:
URL: https://github.com/apache/kafka/pull/14632#discussion_r1371629188
##########
core/src/main/scala/kafka/admin/ConfigCommand.scala:
##########
@@ -445,6 +446,21 @@ object ConfigCommand extends Logging {
if (unknownConfigs.nonEmpty)
throw new IllegalArgumentException(s"Only connection quota configs can be added for '${ConfigType.Ip}' using --bootstrap-server. Unexpected config names: ${unknownConfigs.mkString(",")}")
alterQuotaConfigs(adminClient, entityTypes, entityNames, configsToBeAddedMap, configsToBeDeleted)
+ case ConfigType.ClientMetrics =>
+ val oldConfig = getResourceConfig(adminClient, entityTypeHead, entityNameHead, includeSynonyms = false, describeAll = false)
+ .map { entry => (entry.name, entry) }.toMap
+
+ // fail the command if any of the configs to be deleted does not exist
+ val invalidConfigs = configsToBeDeleted.filterNot(oldConfig.contains)
+ if (invalidConfigs.nonEmpty)
+ throw new InvalidConfigurationException(s"Invalid config(s): ${invalidConfigs.mkString(",")}")
Review Comment:
Yes, you are right it's the way it's written in different places.
--
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