junrao commented on code in PR #16183:
URL: https://github.com/apache/kafka/pull/16183#discussion_r1684941966
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -217,9 +217,10 @@ public enum MetadataVersion {
// Add ELR related supports (KIP-966).
IBP_3_9_IV1(22, "3.9", "IV1", true),
- // Introduce version 1 of the GroupVersion feature (KIP-848).
+ // Introduce version 1 of the GroupVersion feature (KIP-848) and
transaction versions 1 and 2 (KIP-890)
IBP_4_0_IV0(23, "4.0", "IV0", false);
+
Review Comment:
extra new line
##########
core/src/test/scala/unit/kafka/server/BrokerFeaturesTest.scala:
##########
@@ -95,10 +97,20 @@ class BrokerFeaturesTest {
val expectedFeatures = Map[String, Short](
MetadataVersion.FEATURE_NAME ->
MetadataVersion.latestTesting().featureLevel(),
+ ServerFeatures.TRANSACTION_VERSION.featureName() ->
ServerFeatures.TRANSACTION_VERSION.latestTesting(),
"kraft.version" -> 0,
"test_feature_1" -> 4,
"test_feature_2" -> 3,
"test_feature_3" -> 7)
assertEquals(expectedFeatures, brokerFeatures.defaultFinalizedFeatures)
}
+
+ @ParameterizedTest
+ @ValueSource(booleans = Array(true, false))
+ def ensureDefaultSupportedFeaturesRangeNotZeroZero(unstableVersionsEnabled:
Boolean): Unit = {
+ val brokerFeatures = BrokerFeatures.createDefault(unstableVersionsEnabled)
+ brokerFeatures.supportedFeatures.features().values().forEach {
supportedVersionRange =>
+ assertFalse(supportedVersionRange.min() == 0 &&
supportedVersionRange.max() == 0)
Review Comment:
Perhaps it's better to only test `supportedVersionRange.max() == 0` since we
want to disallow ranges like (1,0) too? If so, we want to adjust the method
name accordingly. Ditto for a similar test in
[QuorumFeaturesTest.java](https://github.com/apache/kafka/pull/16183/commits/dc34d935673d6c4eb9373dfea092795ce99a1d8e#diff-7160ed0388ecc53d2af1e7b722f59a25f95475cd36baf4a14e0c038a5473ab97).
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -217,9 +217,10 @@ public enum MetadataVersion {
// Add ELR related supports (KIP-966).
IBP_3_9_IV1(22, "3.9", "IV1", true),
- // Introduce version 1 of the GroupVersion feature (KIP-848).
+ // Introduce version 1 of the GroupVersion feature (KIP-848) and
transaction versions 1 and 2 (KIP-890)
Review Comment:
Since group/txn features don't depend on this MV, perhaps changing the
comment to sth like the following?
bootstrap metadata version of version 1 of the GroupVersion feature
(KIP-848) and transaction versions 1 and 2 (KIP-890)
##########
core/src/main/scala/kafka/server/BrokerFeatures.scala:
##########
@@ -90,13 +90,16 @@ object BrokerFeatures extends Logging {
} else {
MetadataVersion.latestProduction.featureLevel
}))
- PRODUCTION_FEATURES.forEach { feature => features.put(feature.featureName,
- new SupportedVersionRange(0,
- if (unstableFeatureVersionsEnabled) {
- feature.latestTesting
- } else {
- feature.latestProduction
- }))
+ PRODUCTION_FEATURES.forEach {
+ feature => {
Review Comment:
We could get rid of the {} here.
--
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
