[GitHub] [kafka] cmccabe commented on pull request #12182: MINOR: Use dynamic metadata.version check for KIP-704

[GitBox <gi...@apache.org>]

cmccabe commented on PR #12182:
URL: https://github.com/apache/kafka/pull/12182#issuecomment-1132189652

   Thanks for the PR, @mumrah .
   
   1. I agree that tagged fields definitely count as new metadata.  `MetadataVersion.IBP_3_2_IV0` is currently marked with `didMetadataChange = false`. If this metadata version did add new metadata (sounds like it did?), this needs to be marked as `true`. Also, we should probably add a very short description of the new fields added in each version, in the comment for the appropriate MetadataVersion. (i.e. "added X field to Y record"). It's good to set this pattern early so others can follow...
   
   2. I would prefer to embed the `MetadataVersion` in each `*Image` (`AclsImage`, `ClusterImage`, etc.) rather than passing it into `write` and other functions. I think it's fundamental to how we interpret the data -- there may be fields that we have to treat slightly differently based on the version (although we want to minimize this as much as we possibly can, of course). This also lets us have invariants in the constructors like AclsImage objects at MV X never have field Y, and so on, which may be nice for unit tests.
   
   3. I think rather than having `MetadataVersionSupplier`, we should just pass `FeatureControlManager` to the constructors of the other Control Managers. This will be useful in the future when we have other feature flags that we want to check. In general I don't see a big benefit to extracting an interface here -- `FeatureControlManager` is very lightweight to create and if you want one for testing, you can just create one. We do this for a lot of other things -- it works very well when you have an object A and B and A makes a lot of calls into B, but B makes none (or very few) into A. This also makes it easier to understand the control flow.


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