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-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
