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 2022/10/12 17:55:24 UTC
[GitHub] [kafka] soarez commented on a diff in pull request #12729: KAFKA-14285: Delete quota node in zookeeper when configs are empty
soarez commented on code in PR #12729:
URL: https://github.com/apache/kafka/pull/12729#discussion_r993735973
##########
core/src/main/scala/kafka/zk/AdminZkClient.scala:
##########
@@ -365,6 +365,29 @@ class AdminZkClient(zkClient: KafkaZkClient) extends Logging {
case ConfigType.Ip => changeIpConfig(entityName, configs)
case _ => throw new IllegalArgumentException(s"$entityType is not a known entityType. Should be one of ${ConfigType.all}")
}
+
+ if ((ConfigType.Client.equals(entityType) || ConfigType.User.equals(entityType) || ConfigType.Ip.equals(entityType)) && configs.isEmpty) {
+ val currPath = ConfigEntityZNode.path(entityType, entityName)
+ if (zkClient.getChildren(currPath).isEmpty) {
+ var pathToDelete = currPath
+ if (isUserClientId) {
+ val user = entityName.substring(0, entityName.indexOf("/"))
+ val clientId = entityName.substring(entityName.lastIndexOf("/") + 1)
+ val clientsPath = ConfigEntityZNode.path(ConfigType.User, user + "/" + ConfigType.Client)
+ val clientsChildren = zkClient.getChildren(clientsPath)
+ if (clientsChildren.size == 1 && clientsChildren.head.equals(clientId)) {
Review Comment:
```suggestion
if (clientsChildren == Seq(clientId)) {
```
##########
core/src/main/scala/kafka/zk/AdminZkClient.scala:
##########
@@ -365,6 +365,29 @@ class AdminZkClient(zkClient: KafkaZkClient) extends Logging {
case ConfigType.Ip => changeIpConfig(entityName, configs)
case _ => throw new IllegalArgumentException(s"$entityType is not a known entityType. Should be one of ${ConfigType.all}")
}
+
+ if ((ConfigType.Client.equals(entityType) || ConfigType.User.equals(entityType) || ConfigType.Ip.equals(entityType)) && configs.isEmpty) {
+ val currPath = ConfigEntityZNode.path(entityType, entityName)
+ if (zkClient.getChildren(currPath).isEmpty) {
+ var pathToDelete = currPath
+ if (isUserClientId) {
+ val user = entityName.substring(0, entityName.indexOf("/"))
+ val clientId = entityName.substring(entityName.lastIndexOf("/") + 1)
+ val clientsPath = ConfigEntityZNode.path(ConfigType.User, user + "/" + ConfigType.Client)
+ val clientsChildren = zkClient.getChildren(clientsPath)
+ if (clientsChildren.size == 1 && clientsChildren.head.equals(clientId)) {
+ pathToDelete = clientsPath
+ val userData = fetchEntityConfig(ConfigType.User, user)
+ val userPath = ConfigEntityZNode.path(ConfigType.User, user)
+ val userChildren = zkClient.getChildren(userPath)
+ if (userData.isEmpty && userChildren.size == 1 && userChildren.head.equals(ConfigType.Client)) {
+ pathToDelete = userPath
+ }
+ }
+ }
+ zkClient.deletePath(pathToDelete)
Review Comment:
Looking at this block of code, there are apparently three different scenarios for the value of `pathToDelete`, from lines:
1. 372 `var pathToDelete = currPath`
2. 379 `pathToDelete = clientsPath`
3. 384 `pathToDelete = userPath`
But following through the test cases we only seem to hit two of those. `testChangeConfigsWithUserAndClientId` hits 3. and all the other current tests stick to 1. Should we have an extra test case to cover scenario 2. or, if that scenario should never happen, should this logic be re-structured?
##########
core/src/main/scala/kafka/zk/AdminZkClient.scala:
##########
@@ -365,6 +365,29 @@ class AdminZkClient(zkClient: KafkaZkClient) extends Logging {
case ConfigType.Ip => changeIpConfig(entityName, configs)
case _ => throw new IllegalArgumentException(s"$entityType is not a known entityType. Should be one of ${ConfigType.all}")
}
+
+ if ((ConfigType.Client.equals(entityType) || ConfigType.User.equals(entityType) || ConfigType.Ip.equals(entityType)) && configs.isEmpty) {
+ val currPath = ConfigEntityZNode.path(entityType, entityName)
+ if (zkClient.getChildren(currPath).isEmpty) {
+ var pathToDelete = currPath
+ if (isUserClientId) {
+ val user = entityName.substring(0, entityName.indexOf("/"))
+ val clientId = entityName.substring(entityName.lastIndexOf("/") + 1)
+ val clientsPath = ConfigEntityZNode.path(ConfigType.User, user + "/" + ConfigType.Client)
+ val clientsChildren = zkClient.getChildren(clientsPath)
+ if (clientsChildren.size == 1 && clientsChildren.head.equals(clientId)) {
+ pathToDelete = clientsPath
+ val userData = fetchEntityConfig(ConfigType.User, user)
+ val userPath = ConfigEntityZNode.path(ConfigType.User, user)
+ val userChildren = zkClient.getChildren(userPath)
+ if (userData.isEmpty && userChildren.size == 1 && userChildren.head.equals(ConfigType.Client)) {
Review Comment:
```suggestion
if (userData.isEmpty && userChildren == Seq(ConfigType.Client)) {
```
##########
core/src/main/scala/kafka/zk/AdminZkClient.scala:
##########
@@ -365,6 +365,29 @@ class AdminZkClient(zkClient: KafkaZkClient) extends Logging {
case ConfigType.Ip => changeIpConfig(entityName, configs)
case _ => throw new IllegalArgumentException(s"$entityType is not a known entityType. Should be one of ${ConfigType.all}")
}
+
+ if ((ConfigType.Client.equals(entityType) || ConfigType.User.equals(entityType) || ConfigType.Ip.equals(entityType)) && configs.isEmpty) {
Review Comment:
Should we add a line or two with a comment explaining why we're adding this change here?
##########
core/src/main/scala/kafka/zk/AdminZkClient.scala:
##########
@@ -365,6 +365,29 @@ class AdminZkClient(zkClient: KafkaZkClient) extends Logging {
case ConfigType.Ip => changeIpConfig(entityName, configs)
case _ => throw new IllegalArgumentException(s"$entityType is not a known entityType. Should be one of ${ConfigType.all}")
}
+
Review Comment:
It seems that the switch/match block above ends up calling `changeEntityConfig` regardless of the entity type. As a final step, `changeEntityConfig` creates a ZK notification. Would it be better to perform the cleanup before the ZK notification is triggered?
--
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