[GitHub] [kafka] rondagostino commented on a diff in pull request #14083: KAFKA-15219: KRaft support for DelegationTokens

["rondagostino (via GitHub)" <gi...@apache.org>]

rondagostino commented on code in PR #14083:
URL: https://github.com/apache/kafka/pull/14083#discussion_r1293962499


##########
metadata/src/test/java/org/apache/kafka/image/DelegationTokenImageTest.java:
##########


Review Comment:
   Can we include `RemoveDelegationTokenRecord` in a test?



##########
metadata/src/main/resources/common/metadata/DelegationTokenRecord.json:
##########
@@ -22,8 +22,10 @@
   "fields": [
     { "name": "Owner", "type": "string", "versions": "0+",
       "about": "The delegation token owner." },
+    { "name": "Requester", "type": "string", "versions": "0+",
+      "about": "The delegation token requester." },

Review Comment:
   Normally we append new stuff at the end, but this record has never been used before, so I'm okay with inserting in the middle in this case.



##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -173,11 +173,14 @@ public enum MetadataVersion {
     // Adds replica epoch to Fetch request (KIP-903).
     IBP_3_5_IV1(10, "3.5", "IV1", false),
 
-    // Support for SCRAM
+    // KRaft support for SCRAM
     IBP_3_5_IV2(11, "3.5", "IV2", true),
 
-    // Remove leader epoch bump when KRaft controller shrinks the ISR (KAFKA-15021)
-    IBP_3_6_IV0(12, "3.6", "IV0", false);
+    // Support for Remove leader epoch bump when KRaft controller shrinks the ISR (KAFKA-15021)
+    IBP_3_6_IV0(12, "3.6", "IV0", false),
+
+    // KRaft support for DelegationTokens

Review Comment:
   `s/DelegationTokens/Delegation Tokens/`



##########
metadata/src/test/java/org/apache/kafka/image/MetadataImageTest.java:
##########


Review Comment:
   `testApplyDelta1()` needs to have `records.addAll(DelegationTokenImageTest.DELTA1_RECORDS);`. It actually concerns me that this test is passing without it.  I added it locally and it still passes.  How could that be?  Something must be wrong somewhere.  Could you take a look?



##########
server-common/src/main/java/org/apache/kafka/server/common/MetadataVersion.java:
##########
@@ -267,6 +270,10 @@ public boolean isLeaderEpochBumpRequiredOnIsrShrink() {
         return !this.isAtLeast(IBP_3_6_IV0);
     }
 
+    public boolean isDelegationTokenSupported() {
+        return this.isAtLeast(IBP_3_6_IV1);
+    }

Review Comment:
   Need a test for this in `MetadataVersionTest`



-- 
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-unsubscribe@kafka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org