kirktrue commented on code in PR #14642:
URL: https://github.com/apache/kafka/pull/14642#discussion_r1373924481
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java:
##########
@@ -105,6 +105,24 @@ public class ConsumerConfig extends AbstractConfig {
public static final String HEARTBEAT_INTERVAL_MS_CONFIG =
CommonClientConfigs.HEARTBEAT_INTERVAL_MS_CONFIG;
private static final String HEARTBEAT_INTERVAL_MS_DOC =
CommonClientConfigs.HEARTBEAT_INTERVAL_MS_DOC;
+ /**
+ * <code>group.protocol</code>
+ */
+ public static final String GROUP_PROTOCOL_CONFIG = "group.protocol";
+ public static final String DEFAULT_GROUP_PROTOCOL = "generic";
+ public static final String GROUP_PROTOCOL_DOC = "The rebalance protocol
consumer should use. We currently " +
+ "support GENERIC or CONSUMER. If CONSUMER is specified, then the
consumer group protocol will be used. " +
Review Comment:
I think we should use the lower-case versions of these strings:
```suggestion
"support \"generic\" or \"consumer\". If \"consumer\" is specified,
then the consumer group protocol will be used. " +
```
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java:
##########
@@ -105,6 +105,24 @@ public class ConsumerConfig extends AbstractConfig {
public static final String HEARTBEAT_INTERVAL_MS_CONFIG =
CommonClientConfigs.HEARTBEAT_INTERVAL_MS_CONFIG;
private static final String HEARTBEAT_INTERVAL_MS_DOC =
CommonClientConfigs.HEARTBEAT_INTERVAL_MS_DOC;
+ /**
+ * <code>group.protocol</code>
+ */
+ public static final String GROUP_PROTOCOL_CONFIG = "group.protocol";
+ public static final String DEFAULT_GROUP_PROTOCOL = "generic";
+ public static final String GROUP_PROTOCOL_DOC = "The rebalance protocol
consumer should use. We currently " +
+ "support GENERIC or CONSUMER. If CONSUMER is specified, then the
consumer group protocol will be used. " +
+ "Otherwise, the generic group protocol will be used.";
+
+ /**
+ * <code>group.remote.assignor</code>
+ */
+ public static final String REMOTE_ASSIGNOR_CONFIG =
"group.remote.assignor";
+ public static final String DEFAULT_REMOTE_ASSIGNOR = null;
+ public static final String REMOTE_ASSIGNOR_DOC = "The server side assignor
to use. It cannot be used in " +
+ "conjunction with <code>group.local.assignor</code>. The group
coordinator will choose the assignor if no " +
Review Comment:
Two questions:
1. Is `group.local.assignor` the same as `group.local.assignors`?
2. If `group.local.assignors` isn't in this PR, should we just omit that
sentence?
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java:
##########
@@ -610,6 +628,18 @@ public class ConsumerConfig extends AbstractConfig {
DEFAULT_ALLOW_AUTO_CREATE_TOPICS,
Importance.MEDIUM,
ALLOW_AUTO_CREATE_TOPICS_DOC)
+ .define(GROUP_PROTOCOL_CONFIG,
+ Type.STRING,
+ DEFAULT_GROUP_PROTOCOL,
+ ConfigDef.CaseInsensitiveValidString
+
.in(Utils.enumOptions(GroupProtocol.class)),
Review Comment:
You need a wider monitor, @philipnee 😛
```suggestion
ConfigDef.CaseInsensitiveValidString.in(Utils.enumOptions(GroupProtocol.class)),
```
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java:
##########
@@ -610,6 +628,18 @@ public class ConsumerConfig extends AbstractConfig {
DEFAULT_ALLOW_AUTO_CREATE_TOPICS,
Importance.MEDIUM,
ALLOW_AUTO_CREATE_TOPICS_DOC)
+ .define(GROUP_PROTOCOL_CONFIG,
+ Type.STRING,
+ DEFAULT_GROUP_PROTOCOL,
+ ConfigDef.CaseInsensitiveValidString
+
.in(Utils.enumOptions(GroupProtocol.class)),
Review Comment:
You need a wider monitor, @philipnee 😛
```suggestion
ConfigDef.CaseInsensitiveValidString.in(Utils.enumOptions(GroupProtocol.class)),
```
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java:
##########
@@ -105,6 +105,24 @@ public class ConsumerConfig extends AbstractConfig {
public static final String HEARTBEAT_INTERVAL_MS_CONFIG =
CommonClientConfigs.HEARTBEAT_INTERVAL_MS_CONFIG;
private static final String HEARTBEAT_INTERVAL_MS_DOC =
CommonClientConfigs.HEARTBEAT_INTERVAL_MS_DOC;
+ /**
+ * <code>group.protocol</code>
+ */
+ public static final String GROUP_PROTOCOL_CONFIG = "group.protocol";
+ public static final String DEFAULT_GROUP_PROTOCOL = "generic";
Review Comment:
May I suggest:
```suggestion
public static final String DEFAULT_GROUP_PROTOCOL =
GroupProtocol.GENERIC.name().toLowerCase();
```
?
##########
clients/src/main/java/org/apache/kafka/clients/consumer/ConsumerConfig.java:
##########
@@ -610,6 +628,18 @@ public class ConsumerConfig extends AbstractConfig {
DEFAULT_ALLOW_AUTO_CREATE_TOPICS,
Importance.MEDIUM,
ALLOW_AUTO_CREATE_TOPICS_DOC)
+ .define(GROUP_PROTOCOL_CONFIG,
+ Type.STRING,
+ DEFAULT_GROUP_PROTOCOL,
+ ConfigDef.CaseInsensitiveValidString
+
.in(Utils.enumOptions(GroupProtocol.class)),
+ Importance.HIGH,
+ GROUP_PROTOCOL_DOC)
+ .define(REMOTE_ASSIGNOR_CONFIG,
+ Type.STRING,
+ DEFAULT_REMOTE_ASSIGNOR,
+ Importance.MEDIUM,
+ REMOTE_ASSIGNOR_DOC)
Review Comment:
nit: It looks like the indentation is off by four spaces compared to the
rest of the code.
--
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
