You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "dajac (via GitHub)" <gi...@apache.org> on 2024/05/29 11:39:20 UTC
[PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
dajac opened a new pull request, #16120:
URL: https://github.com/apache/kafka/pull/16120
This patch builds on top of https://github.com/apache/kafka/pull/15685. It introduces the `group.version` feature flag with two versions:
1) Version 1 (the default) enables all the features prior to KIP-848 (e.g. classic protocol);
2) Version 2 enables the new consumer group rebalance protocol (KIP-848).
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2142726604
Failures look like usual suspects, but can we confirm the 4.0 ones?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619235007
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala:
##########
@@ -36,7 +37,7 @@ import scala.jdk.CollectionConverters._
class ConsumerGroupHeartbeatRequestTest(cluster: ClusterInstance) {
@ClusterTest()
- def testConsumerGroupHeartbeatIsAccessibleWhenEnabled(): Unit = {
+ def testConsumerGroupHeartbeatIsInaccessibleWhenDisabledByStaticConfig(): Unit = {
Review Comment:
I see your answer below :)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2140752725
The build does not seem to start⦠I am not sure why.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1621497961
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
Mostly confused because we don't check for the config in kafka apis anymore.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619168959
##########
core/src/test/java/kafka/test/annotation/ClusterFeature.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package kafka.test.annotation;
+
+import org.apache.kafka.server.common.Features;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
Review Comment:
I had to extend the test framework to support setting features. See https://github.com/apache/kafka/pull/16120/files#diff-68a3ea4636720b88bf7cffa347f40af0bd0a51b3256e7ef5aa60deb107dc309eR58 as an example. This class is part of this.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619173821
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -3797,10 +3797,16 @@ class KafkaApis(val requestChannel: RequestChannel,
)
}
+ private def isConsumerGroupProtocolEnabled(): Boolean = {
+ val featureLevel = metadataCache.features().finalizedFeatures().getOrDefault(GroupVersion.FEATURE_NAME, GroupVersion.GV_1.featureLevel)
Review Comment:
I think this is reasonable. One of us should probably update the KIP so there aren't more questions on this π
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619153971
##########
server-common/src/main/java/org/apache/kafka/server/common/GroupVersion.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.common;
+
+import java.util.Collections;
+import java.util.Map;
+
+public enum GroupVersion implements FeatureVersion {
Review Comment:
Yes!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac merged PR #16120:
URL: https://github.com/apache/kafka/pull/16120
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2142740321
It looks like testDescribeQuorumReplicationSuccessfu also fails for other PRs: https://ci-builds.apache.org/blue/organizations/jenkins/Kafka%2Fkafka-pr/detail/PR-16154/1/tests. testTopicPartitionsArgWithInternalIncluded seems to be new.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619161780
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -3797,10 +3797,16 @@ class KafkaApis(val requestChannel: RequestChannel,
)
}
+ private def isConsumerGroupProtocolEnabled(): Boolean = {
+ val featureLevel = metadataCache.features().finalizedFeatures().getOrDefault(GroupVersion.FEATURE_NAME, GroupVersion.GV_1.featureLevel)
Review Comment:
is the default version 1 since we always have some group coordinator?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619175329
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -3797,10 +3797,16 @@ class KafkaApis(val requestChannel: RequestChannel,
)
}
+ private def isConsumerGroupProtocolEnabled(): Boolean = {
+ val featureLevel = metadataCache.features().finalizedFeatures().getOrDefault(GroupVersion.FEATURE_NAME, GroupVersion.GV_1.featureLevel)
Review Comment:
We could also use 0 as the default and 1 for the new protocol. It would also work for me.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619179204
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -3797,10 +3797,16 @@ class KafkaApis(val requestChannel: RequestChannel,
)
}
+ private def isConsumerGroupProtocolEnabled(): Boolean = {
+ val featureLevel = metadataCache.features().finalizedFeatures().getOrDefault(GroupVersion.FEATURE_NAME, GroupVersion.GV_1.featureLevel)
Review Comment:
I really don't have a strong opinion either way π
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1621843638
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
We will eventually remove it for sure. This (private) config is still the one use to determine whether the new group coordinator should be enabled or not. The trick to know is that it is automatically set to true when `consumer` is specified as a protocol. We made this to simplify the early access and the preview.
The handling was a bit messy before this patch as the new protocol was enabled whenever the new coordinator ran. It is better now.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2139365026
Apparently, I have to fix a few others.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1621844225
##########
metadata/src/test/java/org/apache/kafka/controller/QuorumFeaturesTest.java:
##########
@@ -74,7 +74,7 @@ public void testDefaultFeatureMapWithUnstable() {
for (Features feature : Features.PRODUCTION_FEATURES) {
expectedFeatures.put(feature.featureName(), VersionRange.of(
0,
- feature.defaultValue(MetadataVersion.LATEST_PRODUCTION)
+ feature.defaultValue(MetadataVersion.latestTesting())
Review Comment:
Indeed.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619171732
##########
core/src/test/java/kafka/test/annotation/ClusterFeature.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package kafka.test.annotation;
+
+import org.apache.kafka.server.common.Features;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
Review Comment:
Just so I complete my understanding, these classes are for setting up clusters in the tests that use this framework?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "junrao (via GitHub)" <gi...@apache.org>.
junrao commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1624848248
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -202,7 +202,10 @@ public enum MetadataVersion {
IBP_3_7_IV4(19, "3.7", "IV4", false),
// Add ELR related supports (KIP-966).
- IBP_3_8_IV0(20, "3.8", "IV0", true);
+ IBP_3_8_IV0(20, "3.8", "IV0", true),
+
+ // Introduce version 1 of the GroupVersion feature (KIP-848).
+ IBP_4_0_IVO(21, "4.0", "IV0", false);
Review Comment:
@dajac : It seems there is a typo. IBP_4_0_IVO should be IBP_4_0_IV0. The last zero was mis-typed as an O.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1624865613
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -202,7 +202,10 @@ public enum MetadataVersion {
IBP_3_7_IV4(19, "3.7", "IV4", false),
// Add ELR related supports (KIP-966).
- IBP_3_8_IV0(20, "3.8", "IV0", true);
+ IBP_3_8_IV0(20, "3.8", "IV0", true),
+
+ // Introduce version 1 of the GroupVersion feature (KIP-848).
+ IBP_4_0_IVO(21, "4.0", "IV0", false);
Review Comment:
https://github.com/apache/kafka/pull/16181 @junrao Could you please review it?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1621487824
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
do we plan to remove this config value from GroupCoordinatorConfig? I see it was removed from a lot of files, but there are still a few where it is used.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1622699466
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
Hmmm -- do when you say the new group coordinator -- you mean the code and not the protocol.
Do I have this right?
1. If the protocol is consumer -> new coordinator
2. If the config is enabled -> new coordinator
3. If protocol is not consumer && config is not enabled -> old coordinator
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
Hmmm -- so when you say the new group coordinator -- you mean the code and not the protocol.
Do I have this right?
1. If the protocol is consumer -> new coordinator
2. If the config is enabled -> new coordinator
3. If protocol is not consumer && config is not enabled -> old coordinator
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619182154
##########
core/src/test/java/kafka/test/annotation/ClusterFeature.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package kafka.test.annotation;
+
+import org.apache.kafka.server.common.Features;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
Review Comment:
All the changes in `java/kafka/test/**` are basically to enable it.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619202602
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala:
##########
@@ -134,11 +162,17 @@ class ConsumerGroupHeartbeatRequestTest(cluster: ClusterInstance) {
assertEquals(-1, consumerGroupHeartbeatResponse.data.memberEpoch)
}
- @ClusterTest(types = Array(Type.KRAFT), serverProperties = Array(
- new ClusterConfigProperty(key = "group.coordinator.rebalance.protocols", value = "classic,consumer"),
- new ClusterConfigProperty(key = "offsets.topic.num.partitions", value = "1"),
- new ClusterConfigProperty(key = "offsets.topic.replication.factor", value = "1")
- ))
+ @ClusterTest(
Review Comment:
how were we enabling before this change? Just including the protocols config?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2137917799
Changes LGTM so far. We will need to rebase once we get the changes and I will give my final approval then. π
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2142885524
testTopicPartitionsArgWithInternalIncluded does not fail locally. It is also a flaky test.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619182154
##########
core/src/test/java/kafka/test/annotation/ClusterFeature.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package kafka.test.annotation;
+
+import org.apache.kafka.server.common.Features;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
Review Comment:
All the changes in `test/java/kafka/test/**` are basically to enable it.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619180891
##########
core/src/test/java/kafka/test/annotation/ClusterFeature.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package kafka.test.annotation;
+
+import org.apache.kafka.server.common.Features;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
Review Comment:
Correct.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619163912
##########
core/src/test/java/kafka/test/annotation/ClusterFeature.java:
##########
@@ -0,0 +1,33 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package kafka.test.annotation;
+
+import org.apache.kafka.server.common.Features;
+
+import java.lang.annotation.Documented;
+import java.lang.annotation.ElementType;
+import java.lang.annotation.Retention;
+import java.lang.annotation.RetentionPolicy;
+import java.lang.annotation.Target;
+
+@Documented
Review Comment:
Excuse my ignorance, but what is this class for?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619217311
##########
server-common/src/main/java/org/apache/kafka/server/common/GroupVersion.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.common;
+
+import java.util.Collections;
+import java.util.Map;
+
+public enum GroupVersion implements FeatureVersion {
+
+ // Version 1 enables the classic rebalance protocol. This is the default
+ // behavior even if the feature flag is not set.
+ // TODO: Change version to IBP_3_8_IVO, needs unstable feature flag.
+ GV_1(1, MetadataVersion.IBP_3_7_IV0, Collections.emptyMap()),
Review Comment:
This one doesn't need unstable version flag right?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619223864
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala:
##########
@@ -134,11 +162,17 @@ class ConsumerGroupHeartbeatRequestTest(cluster: ClusterInstance) {
assertEquals(-1, consumerGroupHeartbeatResponse.data.memberEpoch)
}
- @ClusterTest(types = Array(Type.KRAFT), serverProperties = Array(
- new ClusterConfigProperty(key = "group.coordinator.rebalance.protocols", value = "classic,consumer"),
- new ClusterConfigProperty(key = "offsets.topic.num.partitions", value = "1"),
- new ClusterConfigProperty(key = "offsets.topic.replication.factor", value = "1")
- ))
+ @ClusterTest(
Review Comment:
Correct. We rely on the protocols configs for the EA.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2139033773
@jolshan I rebased it on top of https://github.com/apache/kafka/pull/16130 and I fixed all the tests that I missed.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1621496853
##########
metadata/src/test/java/org/apache/kafka/controller/QuorumFeaturesTest.java:
##########
@@ -74,7 +74,7 @@ public void testDefaultFeatureMapWithUnstable() {
for (Features feature : Features.PRODUCTION_FEATURES) {
expectedFeatures.put(feature.featureName(), VersionRange.of(
0,
- feature.defaultValue(MetadataVersion.LATEST_PRODUCTION)
+ feature.defaultValue(MetadataVersion.latestTesting())
Review Comment:
Hmm was this just a bug in the test...
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2142730589
Will do when I get back to my computer. They indeed look suspicious.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619234724
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala:
##########
@@ -36,7 +37,7 @@ import scala.jdk.CollectionConverters._
class ConsumerGroupHeartbeatRequestTest(cluster: ClusterInstance) {
@ClusterTest()
- def testConsumerGroupHeartbeatIsAccessibleWhenEnabled(): Unit = {
+ def testConsumerGroupHeartbeatIsInaccessibleWhenDisabledByStaticConfig(): Unit = {
Review Comment:
Correct.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619153380
##########
server-common/src/main/java/org/apache/kafka/server/common/GroupVersion.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.common;
+
+import java.util.Collections;
+import java.util.Map;
+
+public enum GroupVersion implements FeatureVersion {
Review Comment:
Hopefully this class was pretty easy to implement :)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619171189
##########
core/src/main/scala/kafka/server/KafkaApis.scala:
##########
@@ -3797,10 +3797,16 @@ class KafkaApis(val requestChannel: RequestChannel,
)
}
+ private def isConsumerGroupProtocolEnabled(): Boolean = {
+ val featureLevel = metadataCache.features().finalizedFeatures().getOrDefault(GroupVersion.FEATURE_NAME, GroupVersion.GV_1.featureLevel)
Review Comment:
Right. I thought that we could use version 1 as the default version for all the existing APIs (old protocol, etc). If not provided, we can still default to it.
I think that in the KIP, we said that we wanted to use version 0 for this but as 0 also means disabled I found it weird. Let me know what you think.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619224183
##########
server-common/src/main/java/org/apache/kafka/server/common/GroupVersion.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.common;
+
+import java.util.Collections;
+import java.util.Map;
+
+public enum GroupVersion implements FeatureVersion {
+
+ // Version 1 enables the classic rebalance protocol. This is the default
+ // behavior even if the feature flag is not set.
+ // TODO: Change version to IBP_3_8_IVO, needs unstable feature flag.
+ GV_1(1, MetadataVersion.IBP_3_7_IV0, Collections.emptyMap()),
Review Comment:
`IBP_3_7_IV0` does not but I thought that using `IBP_3_8_IVO` would be better.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2142887987
I confirmed that the failed tests are not related. Merging to trunk and to 3.8.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1621009962
##########
server-common/src/main/java/org/apache/kafka/server/common/GroupVersion.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.common;
+
+import java.util.Collections;
+import java.util.Map;
+
+public enum GroupVersion implements FeatureVersion {
+
+ // Version 1 enables the classic rebalance protocol. This is the default
+ // behavior even if the feature flag is not set.
+ GV_1(1, MetadataVersion.IBP_3_8_IV0, Collections.emptyMap()),
Review Comment:
When we bootstrap we technically will not get this version. Does that make sense? I'm wondering if the metadata version should be the bootstrap metadata version since it's a little odd to say the classic protocol is not set by default.
Alternatively we take the version 0 approach.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619220834
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -7029,9 +7039,10 @@ class KafkaApisTest extends Logging {
requestChannelRequest.context,
consumerGroupHeartbeatRequest
)).thenReturn(future)
- kafkaApis = createKafkaApis(overrideProperties = Map(
- GroupCoordinatorConfig.NEW_GROUP_COORDINATOR_ENABLE_CONFIG -> "true"
- ))
+ kafkaApis = createKafkaApis(
+ overrideProperties = Map(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG -> "classic,consumer"),
+ raftSupport = true
Review Comment:
why do we need the raft support now?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1622739622
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
Got it. Let me run through the tests one more time with this understanding.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1622733949
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
Correct!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619241318
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -7029,9 +7039,10 @@ class KafkaApisTest extends Logging {
requestChannelRequest.context,
consumerGroupHeartbeatRequest
)).thenReturn(future)
- kafkaApis = createKafkaApis(overrideProperties = Map(
- GroupCoordinatorConfig.NEW_GROUP_COORDINATOR_ENABLE_CONFIG -> "true"
- ))
+ kafkaApis = createKafkaApis(
+ overrideProperties = Map(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG -> "classic,consumer"),
+ raftSupport = true
Review Comment:
So the default is to use ZK Metadata cache (fow now) π
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1621344907
##########
server-common/src/main/java/org/apache/kafka/server/common/GroupVersion.java:
##########
@@ -0,0 +1,78 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.kafka.server.common;
+
+import java.util.Collections;
+import java.util.Map;
+
+public enum GroupVersion implements FeatureVersion {
+
+ // Version 1 enables the classic rebalance protocol. This is the default
+ // behavior even if the feature flag is not set.
+ GV_1(1, MetadataVersion.IBP_3_8_IV0, Collections.emptyMap()),
Review Comment:
Updated to use the version 0 approach.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1622699466
##########
core/src/test/java/kafka/test/ClusterInstance.java:
##########
@@ -159,9 +158,7 @@ default Set<GroupProtocol> supportedGroupProtocols() {
Set<GroupProtocol> supportedGroupProtocols = new HashSet<>();
supportedGroupProtocols.add(CLASSIC);
- // KafkaConfig#isNewGroupCoordinatorEnabled check both NEW_GROUP_COORDINATOR_ENABLE_CONFIG and GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG
- if (serverProperties.getOrDefault(NEW_GROUP_COORDINATOR_ENABLE_CONFIG, "").equals("true") ||
Review Comment:
Hmmm -- so when you say the new group coordinator -- you mean the code (which handles both protocols) and not the protocol itself.
Do I have this right?
1. If the protocol is consumer -> new coordinator
2. If the config is enabled -> new coordinator
3. If protocol is not consumer && config is not enabled -> old coordinator
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "jolshan (via GitHub)" <gi...@apache.org>.
jolshan commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619201114
##########
core/src/test/scala/unit/kafka/server/ConsumerGroupHeartbeatRequestTest.scala:
##########
@@ -36,7 +37,7 @@ import scala.jdk.CollectionConverters._
class ConsumerGroupHeartbeatRequestTest(cluster: ClusterInstance) {
@ClusterTest()
- def testConsumerGroupHeartbeatIsAccessibleWhenEnabled(): Unit = {
+ def testConsumerGroupHeartbeatIsInaccessibleWhenDisabledByStaticConfig(): Unit = {
Review Comment:
which config is the one that enables it? the `group.coordinator.rebalance.protocols` config?
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on PR #16120:
URL: https://github.com/apache/kafka/pull/16120#issuecomment-2139569531
Ok. I have fixed all the tests, hopefully.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1619225494
##########
core/src/test/scala/unit/kafka/server/KafkaApisTest.scala:
##########
@@ -7029,9 +7039,10 @@ class KafkaApisTest extends Logging {
requestChannelRequest.context,
consumerGroupHeartbeatRequest
)).thenReturn(future)
- kafkaApis = createKafkaApis(overrideProperties = Map(
- GroupCoordinatorConfig.NEW_GROUP_COORDINATOR_ENABLE_CONFIG -> "true"
- ))
+ kafkaApis = createKafkaApis(
+ overrideProperties = Map(GroupCoordinatorConfig.GROUP_COORDINATOR_REBALANCE_PROTOCOLS_CONFIG -> "classic,consumer"),
+ raftSupport = true
Review Comment:
because we use the kraft metadata cache now. i am not really happy with how `createKafkaApis` works but i refrained myself from changing everything in this patch :).
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] KAFKA-16860; [1/2] Introduce group.version feature flag [kafka]
Posted by "dajac (via GitHub)" <gi...@apache.org>.
dajac commented on code in PR #16120:
URL: https://github.com/apache/kafka/pull/16120#discussion_r1624863154
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -202,7 +202,10 @@ public enum MetadataVersion {
IBP_3_7_IV4(19, "3.7", "IV4", false),
// Add ELR related supports (KIP-966).
- IBP_3_8_IV0(20, "3.8", "IV0", true);
+ IBP_3_8_IV0(20, "3.8", "IV0", true),
+
+ // Introduce version 1 of the GroupVersion feature (KIP-848).
+ IBP_4_0_IVO(21, "4.0", "IV0", false);
Review Comment:
Oh, good catch. Let me fix this.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org