KAFKA-18553: Update javadoc and comments of ConfigType #18567
Conversation
github-actions
bot
added
triage
| @@ -31,6 +31,5 @@ public class ConfigType { | |||
| public static final String CLIENT_METRICS = "client-metrics"; | |||
| public static final String GROUP = "groups"; | |||
|
|
|||
| // Do not include ClientMetrics and Groups in `all` as they are not supported on ZK. | |||
There was a problem hiding this comment.
Hmm, does this mean we should change ALL? cc @apoorvmittal10 @AndrewJSchofield
There was a problem hiding this comment.
Perhaps we can eliminate ALL since it's exclusively used by ConfigCommand after the removal of ZooKeeper code
There was a problem hiding this comment.
Why would we remove it? It seems like a useful concept and better placed here than in the command itself.
There was a problem hiding this comment.
I would, in fact, avoid the need for ConfigCommand to do its own definition of the config types the system supports - commands should ideally not have any such logic.
There was a problem hiding this comment.
ConfigCommand use ALL to do pre-check before calling related APIs. Hence, it should be fine to remove the ALL as ConfigCommand eventually throw exception if user-defined string is not a allowed type.
There was a problem hiding this comment.
+1 to replace with current implementation with enum.
There was a problem hiding this comment.
If we agree to convert this class to an enum, I suggest we handle this after #18585 is merged, since ConfigType is widely used by AdminZkClient.
There was a problem hiding this comment.
Making it an enum is fine, but can also be done separately. We can file that as a JIRA if we want to decouple from this PR. The reason why I raised the issue is that by removing the comment that was there, we had lost track of the reason for the unusual behavior (which is no longer needed).
There was a problem hiding this comment.
I see.
Indeed the definition of All has been change after removing zk.
I will file a ticket for converting to enum issue.
There was a problem hiding this comment.
frankvicky
force-pushed
the
KAFKA-18553
branch
3 times, most recently
from
51eaeef to
a0bc2b8
Compare
frankvicky
force-pushed
the
KAFKA-18553
branch
from
a0bc2b8 to
0f20644
Compare
| @@ -31,6 +31,5 @@ public class ConfigType { | |||
| public static final String CLIENT_METRICS = "client-metrics"; | |||
| public static final String GROUP = "groups"; | |||
|
|
|||
| // Do not include ClientMetrics and Groups in `all` as they are not supported on ZK. | |||
| public static final List<String> ALL = Arrays.asList(TOPIC, CLIENT, USER, BROKER, IP); | |||
| public static final List<String> ALL = Arrays.asList(TOPIC, CLIENT, USER, BROKER, IP, CLIENT_METRICS, GROUP); | |||
JIRA: KAFKA-18553 The description of this class is outdated.
frankvicky
force-pushed
the
KAFKA-18553
branch
from
0f20644 to
6eec6e9
Compare
JIRA: KAFKA-18553
The description of this class is outdated.
Committer Checklist (excluded from commit message)