cmccabe commented on code in PR #12513: URL: https://github.com/apache/kafka/pull/12513#discussion_r949693026
########## metadata/src/test/java/org/apache/kafka/controller/FeatureControlManagerTest.java: ########## @@ -222,52 +227,130 @@ public void testFeatureControlIterator() throws Exception { manager.iterator(Long.MAX_VALUE)); } + private static final FeatureControlManager.Builder TEST_MANAGER_BUILDER1 = + new FeatureControlManager.Builder(). + setQuorumFeatures(features(MetadataVersion.FEATURE_NAME, + MetadataVersion.IBP_3_3_IV0.featureLevel(), MetadataVersion.IBP_3_3_IV3.featureLevel())). + setMetadataVersion(MetadataVersion.IBP_3_3_IV2); + @Test public void testApplyMetadataVersionChangeRecord() { - QuorumFeatures features = features(MetadataVersion.FEATURE_NAME, - MetadataVersion.IBP_3_0_IV1.featureLevel(), MetadataVersion.IBP_3_3_IV0.featureLevel()); - FeatureControlManager manager = new FeatureControlManager.Builder(). - setQuorumFeatures(features).build(); + FeatureControlManager manager = TEST_MANAGER_BUILDER1.build(); manager.replay(new FeatureLevelRecord(). setName(MetadataVersion.FEATURE_NAME). - setFeatureLevel(MetadataVersion.IBP_3_0_IV1.featureLevel())); - assertEquals(MetadataVersion.IBP_3_0_IV1, manager.metadataVersion()); + setFeatureLevel(MetadataVersion.IBP_3_3_IV3.featureLevel())); + assertEquals(MetadataVersion.IBP_3_3_IV3, manager.metadataVersion()); } @Test - public void testDowngradeMetadataVersion() { - QuorumFeatures features = features(MetadataVersion.FEATURE_NAME, - MetadataVersion.IBP_3_2_IV0.featureLevel(), MetadataVersion.IBP_3_3_IV0.featureLevel()); - FeatureControlManager manager = new FeatureControlManager.Builder(). - setQuorumFeatures(features). - setMetadataVersion(MetadataVersion.IBP_3_3_IV0). - build(); - assertEquals(manager.metadataVersion(), MetadataVersion.IBP_3_3_IV0); + public void testCannotDowngradeToVersionBeforeMinimumSupportedKraftVersion() { + FeatureControlManager manager = TEST_MANAGER_BUILDER1.build(); + assertEquals(ControllerResult.of(Collections.emptyList(), + singletonMap(MetadataVersion.FEATURE_NAME, new ApiError(Errors.INVALID_UPDATE_VERSION, Review Comment: Hmm... I definitely disagree about INVALID_REQUEST. That error code is supposed to be a catch-all for requests that are malformed. I don't think we should be using it in APIs to indicate that the user messed up. In theory a well-programmed client should not send invalid requests. I'm used to the way errno works in C, where there's a small number of error codes and they're heavily overloaded. For example, a bunch of APIs return ENOENT if the entity they're looking for doesn't exist, or EEXIST if it does and that's a problem. I sort of look askance at Kafka's approach. Like did we really need a separate error code for DELEGATION_TOKEN_NOT_FOUND, GROUP_ID_NOT_FOUND, and RESOURCE_NOT_FOUND? Or could we just have a NOT_FOUND error code and return it where appropriate. It's not like you are going to get back DELEGATION_TOKEN_NOT_FOUND from an API that deals with groups, so what's the issue? You are not adding more descriptive power by having an X_NOT_FOUND, Y_NOT_FOUND, Z_NOT_FOUND If only one of them can be used in any given scenario. -- 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