soarez commented on code in PR #14290:
URL: https://github.com/apache/kafka/pull/14290#discussion_r1348135503


##########
generator/src/main/java/org/apache/kafka/message/FieldSpec.java:
##########
@@ -157,11 +157,6 @@ private void checkTagInvariants() {
                     this.taggedVersions + ", which is not open-ended.  
taggedVersions must " +
                     "be either none, or an open-ended range (that ends with a 
plus sign).");
             }
-            if 
(!this.taggedVersions.intersect(this.versions).equals(this.taggedVersions)) {
-                throw new RuntimeException("Field " + name + " specifies 
taggedVersions " +
-                    this.taggedVersions + ", and versions " + this.versions + 
".  " +
-                    "taggedVersions must be a subset of versions.");
-            }

Review Comment:
   > It would be good to know that all v1 PartitionChangeRecords must specify a 
storage directory array if they specify a replica array.
   
   If the new field is specified — named `Assignment` (`[]ReplicaAssignment`), 
replacing `Replicas` (`[]int32`) — it must include `Broker` (`int32`) and 
`Directory` (`uuid`) which is not a tagged field, so it must be specified.
   
   > I think the code that throws an exception if taggedVersions has an end 
version is somewhere else...
   
   It makes sense to keep that, and the check you are referring to is still 
there: 
https://github.com/apache/kafka/blob/trunk/generator/src/main/java/org/apache/kafka/message/FieldSpec.java#L155-L159
   
   > the code above is enforcing that "taggedVersions must be a subset of 
versions" which does seem like an invariant we still want.
   
   "taggedVersions must be a subset of versions"  is essentially two checks:
   
   1. "lowest _taggedVersion_  >= lowest _version_"; and
   2. "highest _taggedVersion_ is <= highest _version_"
   
   Check 2. is at odds with the requirement that _taggedVersions_ must not have 
an end version. But we can keep the first check. Instead of removing the code 
above, we can change it so that we verify that the lowest _tagged version_ is 
equal to or lower than the _version_ e.g.
   
   ```java
   if (this.taggedVersions.lowest() > this.versions.highest()) {
       throw new RuntimeException("Field " + name + " specifies taggedVersions 
" +
               this.taggedVersions + ", and versions " + this.versions + ".  " +
               "The lowest taggedVersion must be lower or equal to the highest 
version.");
   }
   ```
   
   WDYT?
   



##########
generator/src/main/java/org/apache/kafka/message/FieldSpec.java:
##########
@@ -157,11 +157,6 @@ private void checkTagInvariants() {
                     this.taggedVersions + ", which is not open-ended.  
taggedVersions must " +
                     "be either none, or an open-ended range (that ends with a 
plus sign).");
             }
-            if 
(!this.taggedVersions.intersect(this.versions).equals(this.taggedVersions)) {
-                throw new RuntimeException("Field " + name + " specifies 
taggedVersions " +
-                    this.taggedVersions + ", and versions " + this.versions + 
".  " +
-                    "taggedVersions must be a subset of versions.");
-            }

Review Comment:
   > It would be good to know that all v1 PartitionChangeRecords must specify a 
storage directory array if they specify a replica array.
   
   If the new field is specified — named `Assignment` (`[]ReplicaAssignment`), 
replacing `Replicas` (`[]int32`) — it must include `Broker` (`int32`) and 
`Directory` (`uuid`) which is not a tagged field, so it must be specified.
   
   > I think the code that throws an exception if taggedVersions has an end 
version is somewhere else...
   
   It makes sense to keep that, and the check you are referring to is still 
there: 
https://github.com/apache/kafka/blob/trunk/generator/src/main/java/org/apache/kafka/message/FieldSpec.java#L155-L159
   
   > the code above is enforcing that "taggedVersions must be a subset of 
versions" which does seem like an invariant we still want.
   
   "taggedVersions must be a subset of versions"  is essentially two checks:
   
   1. "lowest _taggedVersion_  >= lowest _version_"; and
   2. "highest _taggedVersion_ is <= highest _version_"
   
   Check 2. is at odds with the requirement that _taggedVersions_ must not have 
an end version. But we can keep the first check. Instead of removing the code 
above, we can change it so that we verify that the lowest _tagged version_ is 
equal to or lower than the _version_ e.g.
   
   ```java
   if (this.taggedVersions.lowest() > this.versions.highest()) {
       throw new RuntimeException("Field " + name + " specifies taggedVersions 
" +
               this.taggedVersions + ", and versions " + this.versions + ".  " +
               "The lowest taggedVersion must be lower or equal to the highest 
version.");
   }
   ```
   
   WDYT?
   



-- 
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

Reply via email to