Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[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 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 Type of entity
(topics/clients/users/brokers/broker-
loggers/ips/client-metrics)
--entity-name 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=6,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=6 sensitive=false synonyms={}
match=client_software_name=kafka.python,client_software_version=1\.2\..*
sensitive=false synonyms={}
metrics=org.apache.kafka.consumer. sensiti
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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("/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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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("/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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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("/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("/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",
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] KAFKA-15681: Add support of client-metrics in kafka-configs.sh (KIP-714) [kafka]
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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[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 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 Type of entity
(topics/clients/users/brokers/broker-
loggers/ips/client-metrics)
--entity-name 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=6,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=6 sensitive=false synonyms={}
match=client_software_name=kafka.python,client_software_version=1\.2\..*
sensitive=false synonyms={}
metrics=org.apache.kafka.consumer. sensiti
