You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/12/02 16:34:09 UTC
[GitHub] [kafka] dajac commented on a change in pull request #9628: KAFKA-10747: Implement APIs for altering and describing IP connection rate quotas
dajac commented on a change in pull request #9628:
URL: https://github.com/apache/kafka/pull/9628#discussion_r534287486
##########
File path: core/src/main/scala/kafka/server/AdminManager.scala
##########
@@ -920,32 +954,49 @@ class AdminManager(val config: KafkaConfig,
!name.isDefined || !strict
}
- def fromProps(props: Map[String, String]): Map[String, Double] = {
- props.map { case (key, value) =>
- val doubleValue = try value.toDouble catch {
- case _: NumberFormatException =>
- throw new IllegalStateException(s"Unexpected client quota configuration value: $key -> $value")
- }
- key -> doubleValue
- }
- }
-
- (userEntries ++ clientIdEntries ++ bothEntries).map { case ((u, c), p) =>
+ (userEntries ++ clientIdEntries ++ bothEntries).flatMap { case ((u, c), p) =>
val quotaProps = p.asScala.filter { case (key, _) => QuotaConfigs.isQuotaConfig(key) }
if (quotaProps.nonEmpty && matches(userComponent, u) && matches(clientIdComponent, c))
Some(userClientIdToEntity(u, c) -> fromProps(quotaProps))
else
None
- }.flatten.toMap
+ }.toMap
+ }
+
+ def handleDescribeIpQuotas(ipComponent: Option[ClientQuotaFilterComponent], strict: Boolean): Map[ClientQuotaEntity, Map[String, Double]] = {
+ val ip = ipComponent.flatMap(c => toOption(c.`match`))
+ val exactIp = wantExact(ipComponent)
+ val allIps = ipComponent.exists(_.`match` == null) || (ipComponent.isEmpty && !strict)
+ val ipEntries = if (exactIp)
+ Map(Some(ip.get) -> adminZkClient.fetchEntityConfig(ConfigType.Ip, sanitized(ip)))
+ else if (allIps)
+ adminZkClient.fetchAllEntityConfigs(ConfigType.Ip).map { case (name, props) =>
+ Some(desanitizeEntityName(name)) -> props
+ }
+ else
+ Map.empty
+
+ def ipToQuotaEntity(ip: Option[String]): ClientQuotaEntity = {
+ new ClientQuotaEntity(ip.map(ipName => ClientQuotaEntity.IP -> ipName).toMap.asJava)
+ }
+
+ ipEntries.flatMap { case (ip, props) =>
+ val ipQuotaProps = props.asScala.filter { case (key, _) => DynamicConfig.Ip.names.contains(key) }
+ if (ipQuotaProps.nonEmpty)
+ Some(ipToQuotaEntity(ip) -> fromProps(ipQuotaProps))
+ else
+ None
+ }
}
def alterClientQuotas(entries: Seq[ClientQuotaAlteration], validateOnly: Boolean): Map[ClientQuotaEntity, ApiError] = {
def alterEntityQuotas(entity: ClientQuotaEntity, ops: Iterable[ClientQuotaAlteration.Op]): Unit = {
- val (path, configType, configKeys) = entityToSanitizedUserClientId(entity) match {
- case (Some(user), Some(clientId)) => (user + "/clients/" + clientId, ConfigType.User, DynamicConfig.User.configKeys)
- case (Some(user), None) => (user, ConfigType.User, DynamicConfig.User.configKeys)
- case (None, Some(clientId)) => (clientId, ConfigType.Client, DynamicConfig.Client.configKeys)
- case _ => throw new InvalidRequestException("Invalid empty client quota entity")
+ val (path, configType, configKeys) = parseAndSanitizeQuotaEntity(entity) match {
+ case (Some(user), Some(clientId), None) => (user + "/clients/" + clientId, ConfigType.User, DynamicConfig.User.configKeys)
+ case (Some(user), None, None) => (user, ConfigType.User, DynamicConfig.User.configKeys)
+ case (None, Some(clientId), None) => (clientId, ConfigType.Client, DynamicConfig.Client.configKeys)
+ case (None, None, Some(ip)) => (ip, ConfigType.Ip, DynamicConfig.Ip.configKeys)
+ case _ => throw new InvalidRequestException("Invalid client quota entity")
Review comment:
Should we also provide a better error message if the user mixes up IP and user/clientId?
##########
File path: core/src/main/scala/kafka/server/AdminManager.scala
##########
@@ -920,32 +954,49 @@ class AdminManager(val config: KafkaConfig,
!name.isDefined || !strict
}
- def fromProps(props: Map[String, String]): Map[String, Double] = {
- props.map { case (key, value) =>
- val doubleValue = try value.toDouble catch {
- case _: NumberFormatException =>
- throw new IllegalStateException(s"Unexpected client quota configuration value: $key -> $value")
- }
- key -> doubleValue
- }
- }
-
- (userEntries ++ clientIdEntries ++ bothEntries).map { case ((u, c), p) =>
+ (userEntries ++ clientIdEntries ++ bothEntries).flatMap { case ((u, c), p) =>
val quotaProps = p.asScala.filter { case (key, _) => QuotaConfigs.isQuotaConfig(key) }
if (quotaProps.nonEmpty && matches(userComponent, u) && matches(clientIdComponent, c))
Some(userClientIdToEntity(u, c) -> fromProps(quotaProps))
else
None
- }.flatten.toMap
+ }.toMap
Review comment:
I am not sure to understand why you have removed `flatten` here. Could you elaborate a bit on this please?
##########
File path: core/src/main/scala/kafka/admin/ConfigCommand.scala
##########
@@ -56,11 +56,12 @@ import scala.collection._
* --entity-type users --entity-name <user-principal> --entity-type clients --entity-name <client-id>
* <li> broker: --broker <broker-id> OR --entity-type brokers --entity-name <broker-id>
* <li> broker-logger: --broker-logger <broker-id> OR --entity-type broker-loggers --entity-name <broker-id>
+ * <li> ip: --IP <ip> OR --entity-type IPs --entity-name <ip>
* </ul>
- * --entity-type <users|clients|brokers> --entity-default may be specified in place of --entity-type <users|clients|brokers> --entity-name <entityName>
- * when describing or altering default configuration for users, clients, or brokers, respectively.
- * Alternatively, --user-defaults, --client-defaults, or --broker-defaults may be specified in place of
- * --entity-type <users|clients|brokers> --entity-default, respectively.
+ * --entity-type <users|clients|brokers|IPs> --entity-default may be specified in place of --entity-type <users|clients|brokers|IPs> --entity-name <entityName>
+ * when describing or altering default configuration for users, clients, brokers, or IPs, respectively.
+ * Alternatively, --user-defaults, --client-defaults, --broker-defaults, or --IP-defaults may be specified in place of
Review comment:
`--IP-defaults` looks pretty bad. I think that we should definitely use lower-case.
##########
File path: clients/src/main/java/org/apache/kafka/common/quota/ClientQuotaEntity.java
##########
@@ -32,6 +32,7 @@
*/
public static final String USER = "user";
public static final String CLIENT_ID = "client-id";
+ public static final String IP = "IP";
Review comment:
I would also make it lower-case for consistency.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org