MINOR: Adds entity-specific flags to ConfigCommand per KIP-543.#7667
Conversation
| else | ||
| None | ||
| val name = entityNames.headOption match { | ||
| case Some("") => Some(ConfigEntityName.Default) |
There was a problem hiding this comment.
I know this is an existing problem in the code, but I really think we should avoid overloading the meaning of the empty string to mean "the default." It's not very clear, and furthermore, we made the unfortunate decision to allow empty names in some cases. For example empty consumer group names are valid. :( Let's use a List[Option[String]] here to make it clear.
There was a problem hiding this comment.
Agreed, this would be much cleaner. The only concern I have is that this may break compatibility, e.g. --entity-type users --entity-name "" (double quotes) would resolve to the default user today, whereas with this change it'd resolve to the empty user name. If we continue to pass it along, then the formed ZK config path may be invalid/unexpected.
I would consider leaving it as-is for ConfigCommand, and then the new API can be more intelligent about this sort of thing. Thoughts?
There was a problem hiding this comment.
Ping - any thoughts/comments? BTW, you will need to integrate the PR since I don't have permissions.
There was a problem hiding this comment.
I guess we can kick the can down the road with regard to empty string handling. We will have to tackle it when designing the client side quotas API, though (probably by forbidding empty strings for now...)
|
Thanks, @bdbyrne. Looks good. |
|
LGTM |
…gCommand per KIP-543. (apache#7667) Reviewers: Colin P. McCabe <cmccabe@apache.org>
Committer Checklist (excluded from commit message)