artemlivshits commented on code in PR #15685:
URL: https://github.com/apache/kafka/pull/15685#discussion_r1591704034
##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -258,6 +259,13 @@ public enum MetadataVersion {
this.didMetadataChange = didMetadataChange;
}
+ public String featureName() {
+ return FEATURE_NAME;
+ }
+
+ public void validateVersion(MetadataVersion metadataVersion,
List<FeatureVersion> features) {
+ }
Review Comment:
Could we add a comment on why validation is not needed?
##########
clients/src/main/java/org/apache/kafka/common/feature/BaseVersionRange.java:
##########
@@ -26,7 +26,7 @@
/**
* Represents an immutable basic version range using 2 attributes: min and
max, each of type short.
* The min and max attributes need to satisfy 2 rules:
- * - they are each expected to be >= 1, as we only consider positive version
values to be valid.
+ * - they are each expected to be >= 0, as we only consider positive version
values to be valid.
Review Comment:
Nit: to be precise we should change 'positive' -> 'non-negative' :-)
##########
server-common/src/main/java/org/apache/kafka/server/common/TestFeatureVersion.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.List;
+
+public enum TestFeatureVersion implements FeatureVersion {
+
+ TEST_0(0),
+ TEST_1(1),
+ TEST_2(2);
+
+ private short featureLevel;
+
+ public static final String FEATURE_NAME = "test.feature.version";
+ public static final TestFeatureVersion PRODUCTION_VERSION = TEST_1;
+
+ TestFeatureVersion(int featureLevel) {
+ this.featureLevel = (short) featureLevel;
+ }
Review Comment:
I wonder if instead of doing manual implementation of validation and
mapping, we could have minMetadataVersion as an enum argument, then validation
would be just comparison, and for mapping we'd need to enumerate in reverse
order and pick the first we find that supports the metadata version.
##########
server-common/src/main/java/org/apache/kafka/server/common/FeatureVersion.java:
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.Arrays;
+import java.util.List;
+import java.util.Optional;
+
+/**
+ * This is an interface for the various features implemented for Kafka
clusters.
+ * KIP-584: Versioning Scheme for Features introduced the idea of various
features, but only added one feature -- MetadataVersion.
+ * KIP-1022: Formatting and Updating Features allowed for more features to be
added. In order to set and update features,
+ * they need to be specified via the StorageTool or FeatureCommand tools.
+ * <br>
+ * Having a unified interface for the features that will use a shared type in
the API used to set and update them
+ * makes it easier to process these features.
+ */
+public interface FeatureVersion {
+
+ /**
+ * Features currently used in production. If a feature is included in this
list, it will also be specified when
+ * formatting a cluster via the StorageTool. MetadataVersion is handled
separately, so it is not included here.
+ *
+ * When a feature is added here, make sure it has a mapping in {@link
#defaultValue} and {@link #createFeature}.
+ * See {@link TestFeatureVersion} as an example.
+ */
+ List<String> PRODUCTION_FEATURES = Arrays.asList();
Review Comment:
Instead of having a list of strings and then manually adding code into
defaultValue and createFeature, can we do something similar to what Errors.java
is doing? Each feature would be an enum value with the name and a builder
function that would create the concrete feature object. Then adding a new
feature would be declarative and would just get added to enum.
##########
server-common/src/main/java/org/apache/kafka/server/common/TestFeatureVersion.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.List;
+
+public enum TestFeatureVersion implements FeatureVersion {
+
+ TEST_0(0),
+ TEST_1(1),
+ TEST_2(2);
+
+ private short featureLevel;
+
+ public static final String FEATURE_NAME = "test.feature.version";
+ public static final TestFeatureVersion PRODUCTION_VERSION = TEST_1;
+
+ TestFeatureVersion(int featureLevel) {
+ this.featureLevel = (short) featureLevel;
+ }
+
+ public short featureLevel() {
+ return featureLevel;
+ }
+
+ public String featureName() {
+ return FEATURE_NAME;
+ }
+
+ public void validateVersion(MetadataVersion metadataVersion,
List<FeatureVersion> features) {
+ // version 1 depends on metadata.version 3.3-IVO
+ if (featureLevel >= 1 &&
metadataVersion.isLessThan(MetadataVersion.IBP_3_3_IV0))
+ throw new IllegalArgumentException(FEATURE_NAME + " could not be
set to " + featureLevel +
+ " because it depends on metadata.version=14 (" +
MetadataVersion.IBP_3_3_IV0 + ")");
+ }
+
+ public static TestFeatureVersion metadataVersionMapping(MetadataVersion
metadataVersion) {
+ if (metadataVersion.isLessThan(MetadataVersion.IBP_3_8_IV0)) {
Review Comment:
Validation uses `IBP_3_3_IV0` and mapping uses `IBP_3_8_IV0` version.
--
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
