[GitHub] [kafka] cmccabe commented on a diff in pull request #12513: MINOR: some fixes to the KRaft bootstrap logic

[GitBox]

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 have a different view about INVALID_REQUEST. I thought that error 
code was supposed to be a catch-all for requests that were 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