chia7712 commented on code in PR #18641:
URL: https://github.com/apache/kafka/pull/18641#discussion_r1938724850
##########
core/src/main/scala/kafka/server/DynamicBrokerConfig.scala:
##########
@@ -48,13 +48,6 @@ import scala.collection._
import scala.jdk.CollectionConverters._
/**
- * Dynamic broker configurations are stored in ZooKeeper and may be defined
at two levels:
Review Comment:
in kraft mode, the two levels are still existent. could you please revise it
instead of removing it?
##########
core/src/main/scala/kafka/server/DynamicConfig.scala:
##########
@@ -85,12 +85,10 @@ object DynamicConfig {
def validate(props: Properties): util.Map[String, AnyRef] =
DynamicConfig.validate(ipConfigs, props, customPropsAllowed = false)
def isValidIpEntity(ip: String): Boolean = {
Review Comment:
this is used by `ConfigCommand` only, so could you please move this method
to `ConfigCommand`?
##########
core/src/main/scala/kafka/server/metadata/ClientQuotaMetadataManager.scala:
##########
@@ -145,27 +149,43 @@ class ClientQuotaMetadataManager(private[metadata] val
quotaManagers: QuotaManag
}
// Convert entity into Options with sanitized values for QuotaManagers
- val (sanitizedUser, sanitizedClientId) = quotaEntity match {
- case UserEntity(user) => (Some(Sanitizer.sanitize(user)), None)
- case DefaultUserEntity => (Some(ZooKeeperInternals.DEFAULT_STRING), None)
- case ClientIdEntity(clientId) => (None,
Some(Sanitizer.sanitize(clientId)))
- case DefaultClientIdEntity => (None,
Some(ZooKeeperInternals.DEFAULT_STRING))
- case ExplicitUserExplicitClientIdEntity(user, clientId) =>
(Some(Sanitizer.sanitize(user)), Some(Sanitizer.sanitize(clientId)))
- case ExplicitUserDefaultClientIdEntity(user) =>
(Some(Sanitizer.sanitize(user)), Some(ZooKeeperInternals.DEFAULT_STRING))
- case DefaultUserExplicitClientIdEntity(clientId) =>
(Some(ZooKeeperInternals.DEFAULT_STRING), Some(Sanitizer.sanitize(clientId)))
- case DefaultUserDefaultClientIdEntity =>
(Some(ZooKeeperInternals.DEFAULT_STRING),
Some(ZooKeeperInternals.DEFAULT_STRING))
- case IpEntity(_) | DefaultIpEntity => throw new
IllegalStateException("Should not see IP quota entities here")
- }
+ val (userEntity, sanitizedClientEntity) =
transferToClientQuotaEntity(quotaEntity)
val quotaValue = newValue.map(new Quota(_, true))
try {
manager.updateQuota(
- sanitizedUser = sanitizedUser,
- clientId = sanitizedClientId.map(Sanitizer.desanitize),
- sanitizedClientId = sanitizedClientId,
- quota = quotaValue)
+ userEntity = userEntity,
+ sanitizedClientEntity = sanitizedClientEntity,
+ quota = quotaValue
+ )
} catch {
case t: Throwable => error(s"Failed to update user-client quota
$quotaEntity", t)
}
}
}
+
+object ClientQuotaMetadataManager {
+
+ def transferToClientQuotaEntity(quotaEntity: QuotaEntity):
(Option[BaseUserEntity], Option[ClientQuotaConfigEntity]) = {
+ val (userEntity, sanitizedClientEntity) = quotaEntity match {
Review Comment:
it is unnecessary to declare `val (userEntity, sanitizedClientEntity)`
```scala
quotaEntity match {
case UserEntity(user) =>
(Some(ClientQuotaManager.UserEntity(Sanitizer.sanitize(user))), None)
case DefaultUserEntity =>
(Some(ClientQuotaManager.DefaultUserEntity), None)
case ClientIdEntity(clientId) =>
(None,
Some(ClientQuotaManager.ClientIdEntity(Sanitizer.sanitize(clientId))))
case DefaultClientIdEntity =>
(None, Some(ClientQuotaManager.DefaultClientIdEntity))
case ExplicitUserExplicitClientIdEntity(user, clientId) =>
(Some(ClientQuotaManager.UserEntity(Sanitizer.sanitize(user))),
Some(ClientQuotaManager.ClientIdEntity(Sanitizer.sanitize(clientId))))
case ExplicitUserDefaultClientIdEntity(user) =>
(Some(ClientQuotaManager.UserEntity(Sanitizer.sanitize(user))),
Some(ClientQuotaManager.DefaultClientIdEntity))
case DefaultUserExplicitClientIdEntity(clientId) =>
(Some(ClientQuotaManager.DefaultUserEntity),
Some(ClientQuotaManager.ClientIdEntity(Sanitizer.sanitize(clientId))))
case DefaultUserDefaultClientIdEntity =>
(Some(ClientQuotaManager.DefaultUserEntity),
Some(ClientQuotaManager.DefaultClientIdEntity))
case IpEntity(_) | DefaultIpEntity => throw new
IllegalStateException("Should not see IP quota entities here")
}
```
##########
core/src/main/scala/kafka/server/ClientQuotaManager.scala:
##########
@@ -55,7 +55,7 @@ object QuotaTypes {
object ClientQuotaManager {
// Purge sensors after 1 hour of inactivity
val InactiveSensorExpirationTimeSeconds = 3600
-
+ private val DefaultString = "<default>"
Review Comment:
How about renaming it to `DefaultName`?
--
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
