Skip to content
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.

Sign up for GitHub

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-18553: Update javadoc and comments of ConfigType #18567

Merged
merged 1 commit into from
Jan 20, 2025
+3 −5
Merged

KAFKA-18553: Update javadoc and comments of ConfigType #18567

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion core/src/main/scala/kafka/admin/ConfigCommand.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ object ConfigCommand extends Logging {

private val BrokerDefaultEntityName = ""
val BrokerLoggerConfigType = "broker-loggers"
private val BrokerSupportedConfigTypes = ConfigType.ALL.asScala :+ BrokerLoggerConfigType :+ ConfigType.CLIENT_METRICS :+ ConfigType.GROUP
private val BrokerSupportedConfigTypes = ConfigType.ALL.asScala :+ BrokerLoggerConfigType
private val DefaultScramIterations = 4096

def main(args: Array[String]): Unit = {
Expand Down
6 changes: 2 additions & 4 deletions server-common/src/main/java/org/apache/kafka/server/config/ConfigType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,10 @@
*/
package org.apache.kafka.server.config;

import java.util.Arrays;
import java.util.List;

/**
* Represents all the entities that can be configured via ZK
* Represents all the entities that can be configured.
*/
public class ConfigType {
public static final String TOPIC = "topics";
Expand All @@ -31,6 +30,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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, does this mean we should change ALL? cc @apoorvmittal10 @AndrewJSchofield

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can eliminate ALL since it's exclusively used by ConfigCommand after the removal of ZooKeeper code

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we remove it? It seems like a useful concept and better placed here than in the command itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to replace with current implementation with enum.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see.
Indeed the definition of All has been change after removing zk.
I will file a ticket for converting to enum issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://issues.apache.org/jira/browse/KAFKA-18576

public static final List<String> ALL = Arrays.asList(TOPIC, CLIENT, USER, BROKER, IP);
public static final List<String> ALL = List.of(TOPIC, CLIENT, USER, BROKER, IP, CLIENT_METRICS, GROUP);
}
Loading