artemlivshits commented on code in PR #15685: URL: https://github.com/apache/kafka/pull/15685#discussion_r1610437612
########## server-common/src/main/java/org/apache/kafka/server/common/TestFeatureVersion.java: ########## @@ -0,0 +1,56 @@ +/* + * 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 TestFeatureVersion implements FeatureVersion { + + TEST_0(0, MetadataVersion.IBP_3_3_IV0, Collections.emptyMap()), Review Comment: Version 0 == "feature doesn't exist", does it need to be explicitly codified the enum? ########## metadata/src/main/java/org/apache/kafka/controller/ClusterControlManager.java: ########## @@ -470,8 +471,8 @@ BrokerFeature processRegistrationFeature( // A feature is not found in the finalizedFeature map if it is unknown to the controller or set to 0 (feature not enabled). // As more features roll out, it may be common to leave a feature disabled, so this log is debug level in the case Review Comment: Comment seems to be outdated w.r.t. latest logic? ########## server-common/src/main/java/org/apache/kafka/server/common/FeatureVersion.java: ########## @@ -0,0 +1,49 @@ +/* + * 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.Map; + +public interface FeatureVersion { + + /** + * The level of the feature. 0 means the feature is disabled. + */ + short featureLevel(); + + /** + * The name of the feature. + */ + String featureName(); + + /** + * The minimum metadata version which sets this feature version as default. When bootstrapping using only + * a metadata version, a reasonable default for all other features is chosen based on this value. + * This should be defined as the next metadata version to be released when the feature version becomes production ready. + * (Ie, if the current production MV is 17 when a feature version is released, its mapping should be to MV 18) Review Comment: Can we make it map to the release version? If not, can we add a comment explaining the logic? Intuitively, if we have a feature like - FOO(1, 42) // released version 1 when MV got bumped to 42 - FOO(2, 77) // released version 2 when MV got bumped to 77 - FOO(3, 77) // released version 3 at the same time as 2 we should be able to discover that if we got MV 45, then FOO=1 because 1 was available at 42 and 2&3 are not available yet. If we got 78 then it should be 3 because it was max available at 77. If we got MV 30 then we should get 0 (implicit) because it wasn't available yet. Using the same table, we can determine that if a feature version 1 was selected with MV 41, then it's invalid combination; similarly if feature 2 or 3 was selected with MV 45 it's invalid combination. And we can select 1, 2 or 3 when MV is 77. ########## server-common/src/main/java/org/apache/kafka/server/common/Features.java: ########## @@ -64,14 +63,16 @@ public enum Features { PRODUCTION_FEATURES = Arrays.stream(FEATURES).filter(feature -> feature.usedInProduction).collect(Collectors.toList()); + PRODUCTION_FEATURE_NAMES = PRODUCTION_FEATURES.stream().map(feature -> + feature.name).collect(Collectors.toList()); } public String featureName() { return name; } public FeatureVersion[] features() { Review Comment: Should the method name also reflect that it's versions, not features? -- 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
