artemlivshits commented on code in PR #15685: URL: https://github.com/apache/kafka/pull/15685#discussion_r1612863802
########## server-common/src/main/java/org/apache/kafka/server/common/TestFeatureVersion.java: ########## @@ -21,16 +21,17 @@ public enum TestFeatureVersion implements FeatureVersion { - TEST_0(0, MetadataVersion.IBP_3_3_IV0, Collections.emptyMap()), + // TEST_1 released right before MV 3.7-IVO was released, and it has no dependencies TEST_1(1, MetadataVersion.IBP_3_7_IV0, Collections.emptyMap()), + // TEST_2 released right before MV 3.8-IVO was released, and it depends on this metadata version TEST_2(2, MetadataVersion.IBP_3_8_IV0, Collections.singletonMap(MetadataVersion.FEATURE_NAME, MetadataVersion.IBP_3_8_IV0.featureLevel())); - private short featureLevel; - private MetadataVersion metadataVersionMapping; - private Map<String, Short> dependencies; + private final short featureLevel; + private final MetadataVersion metadataVersionMapping; + private final Map<String, Short> dependencies; public static final String FEATURE_NAME = "test.feature.version"; - public static final TestFeatureVersion PRODUCTION_VERSION = TEST_1; + public static final TestFeatureVersion LATEST_PRODUCTION = TEST_1; Review Comment: Does it mean that the broker will show the test feature to the clients? ########## core/src/main/scala/kafka/tools/StorageTool.scala: ########## @@ -109,6 +111,52 @@ object StorageTool extends Logging { } } + private def validateMetadataVersion(metadataVersion: MetadataVersion, config: Option[KafkaConfig]): Unit = { + if (!metadataVersion.isKRaftSupported) { + throw new TerseFailure(s"Must specify a valid KRaft metadata.version of at least ${MetadataVersion.IBP_3_0_IV0}.") + } + if (!metadataVersion.isProduction) { + if (config.get.unstableMetadataVersionsEnabled) { + System.out.println(s"WARNING: using pre-production metadata.version $metadataVersion.") + } else { + throw new TerseFailure(s"The metadata.version $metadataVersion is not ready for production use yet.") + } + } + } + + private[tools] def generateFeatureRecords(metadataRecords: ArrayBuffer[ApiMessageAndVersion], + metadataVersion: MetadataVersion, + specifiedFeatures: Map[String, java.lang.Short], + allFeatures: List[Features], + usesVersionDefault: Boolean): Unit = { + // If we are using --version-default, the default is based on the metadata version. + val metadataVersionForDefault = if (usesVersionDefault) Optional.of(metadataVersion) else Optional.empty[MetadataVersion]() Review Comment: I think it would be confusing if a user specifies the latest MV version and the result would be different from when nothing is specified (implied assumption is that nothing is shortcut for latest MV known to the tool). It would also be confusing if by default (nothing is specified) we don't have all features set to the latest versions known to the tool. We can provide guidance to new features developers in the comments (and the test feature example) and add a unit test that enforces the equivalence. -- 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