-
Notifications
You must be signed in to change notification settings - Fork 13.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
KAFKA-14285: Delete quota node in zookeeper when configs are empty #12727
Conversation
We should add a test for this PR since it will change the behavior, right? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on adding a test for this
if (props.isEmpty) { | ||
// try to clean empty quota nodes | ||
val currPath = ConfigEntityZNode.path(configType, path) | ||
if (zkClient.getChildren(currPath).isEmpty) { | ||
var pathToDelete = currPath | ||
if (userClientQuota) { | ||
val clientsPath = ConfigEntityZNode.path(ConfigType.User, userOpt.get + "/" + ConfigType.Client) | ||
val clientsChildren = zkClient.getChildren(clientsPath) | ||
if (clientsChildren.size == 1 && clientsChildren.head.equals(clientIdOpt.get)) { | ||
pathToDelete = clientsPath | ||
val userData = adminZkClient.fetchEntityConfig(ConfigType.User, userOpt.get) | ||
val userPath = ConfigEntityZNode.path(ConfigType.User, userOpt.get) | ||
val userChildren = zkClient.getChildren(userPath) | ||
if (userData.isEmpty && userChildren.size == 1 && userChildren.head.equals(ConfigType.Client)) { | ||
pathToDelete = userPath | ||
} | ||
} | ||
} | ||
zkClient.deletePath(pathToDelete) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me that ZkAdminManager
is not the right place for this logic, it might make more sense to have this in AdminZkClient
, perhaps in hangeConfigs()
or changeUserOrUserClientIdConfig()
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with that. It may be better to do this logic in AdminZkClient. I have moved it to AdminZkClient and added corresponding tests.
Thank you.
Right. I have added corresponding tests. |
More detailed description of your change,
if necessary. The PR title and PR message become
the squashed commit message, so use a separate
comment to ping reviewers.
Summary of testing strategy (including rationale)
for the feature or bug fix. Unit and/or integration
tests are expected for any behaviour change and
system tests should be considered for larger changes.
Committer Checklist (excluded from commit message)