Hi Edo, Thanks for the KIP. Looks like a useful improvement. I have some comments/questions.
4) For a new interface, I wonder whether it would be better to use TopicIdPartition rather than TopicPartition. Topic IDs are gradually spreading across the public interfaces for Kafka. 5) The new topic config is called `record.validation.policy`. The javadoc for the validationPolicy() method says `validation.policy`. 6) I’m surprised that you need a `HeaderProxy` interface when `Headers` and `Header` are already interfaces. I would have expected it was possible to create proxy instances of the headers using the existing interfaces with a little cunning. Thanks, Andrew > On 26 Jun 2023, at 16:05, Edoardo Comar <edoardli...@gmail.com> wrote: > > Hi Kirk, > thanks for your comments. > >> 1. Does record.validation.policy.class.name support multiple classes, or >> just one? I’m probably not wrapping my head around it, >> but I’d imagine different policies for different families or groupings of >> topics, thus the need for supporting multiple policies. >> But if there are multiple then you run the risk of conflicts of ownership of >> validation, so ¯\_(ツ)_/¯ > > We have only identified the use case for a single policy, but as > mentioned in the Rejected Alternatives section of the KIP, we think a > Facade-like policy could be written by a Kafka admin to dispatch > validation to different policies. Allowing multiple policies to be > specified would introduce complexities (as you noted), and we wanted > to avoid making too many assumptions without having a strong use-case > for this support. > >> 2. Is there any concern that a validation class may alter the contents of >> the ByteBuffer of the key and/or value? >> Perhaps that’s already handled and/or outside the scope of this KIP? > > Good point. This behaviour isn’t defined, and what happens could > change between Kafka versions (or depending on compression settings). > We have modified the Javadoc in the KIP to indicate that the > ByteBuffer’s are read-only wrapper, as we don’t intend this plug-point > to be used for modifying message data. We also spotted that this was a > problem for returning headers (as the common Header interface returns > a byte array from one of its methods), and have updated the Javadoc > accordingly. > > >> 3. What is the benefit to introducing the inner TopicMetadata and >> RecordProxy interfaces vs. >> passing the TopicPartition, String (validation policy), and Record into the >> validate() method directly? > > We wanted to avoid Record as the package > org.apache.kafka.common.record is documented as *not* being a > published API, and doesn’t necessarily maintain compatibility between > versions. This was highlighted as a potential problem during the > discussion of KIP-729. > > We designed the API using interfaces as arguments to the methods, so > that further properties can be added in the future without breaking > existing implementations. > > > On Wed, 21 Jun 2023 at 17:08, Kirk True <k...@kirktrue.pro> wrote: >> >> Hi Edo/Adrian! >> >> Thanks for the KIP. >> >> I have some questions, and apologies that the may fall under the “stupid” >> column because I’m not that familiar with this area :) >> >> 1. Does record.validation.policy.class.name support multiple classes, or >> just one? I’m probably not wrapping my head around it, but I’d imagine >> different policies for different families or groupings of topics, thus the >> need for supporting multiple policies. But if there are multiple then you >> run the risk of conflicts of ownership of validation, so ¯\_(ツ)_/¯ >> >> 2. Is there any concern that a validation class may alter the contents of >> the ByteBuffer of the key and/or value? Perhaps that’s already handled >> and/or outside the scope of this KIP? >> >> 3. What is the benefit to introducing the inner TopicMetadata and >> RecordProxy interfaces vs. passing the TopicPartition, String (validation >> policy), and Record into the validate() method directly? >> >> Thanks, >> Kirk >> >>> On Jun 20, 2023, at 2:28 AM, Edoardo Comar <edoardli...@gmail.com> wrote: >>> >>> Thanks Николай, >>> We’d like to open a vote next week. >>> Hopefully getting some more feedback before then. >>> >>> Edo >>> >>> >>> On Wed, 7 Jun 2023 at 19:15, Николай Ижиков <nizhi...@apache.org> wrote: >>> >>>> Hello. >>>> >>>> As author of one of related KIPs I’m +1 for this change. >>>> Long waited feature. >>>> >>>>> 7 июня 2023 г., в 19:02, Edoardo Comar <eco...@uk.ibm.com> написал(а): >>>>> >>>>> Dear all, >>>>> Adrian and I would like to start a discussion thread on >>>>> >>>>> KIP-940: Broker extension point for validating record contents at >>>> produce time >>>>> >>>>> >>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-940%3A+Broker+extension+point+for+validating+record+contents+at+produce+time >>>>> >>>>> This KIP proposes a new broker-side extension point (a “record >>>> validation policy”) that can be used to reject records published by a >>>> misconfigured client. >>>>> Though general, it is aimed at the common, best-practice use case of >>>> defining Kafka message formats with schemas maintained in a schema >>>> registry. >>>>> >>>>> Please post your feedback, thanks ! >>>>> >>>>> Edoardo & Adrian >>>>> >>>>> Unless otherwise stated above: >>>>> >>>>> IBM United Kingdom Limited >>>>> Registered in England and Wales with number 741598 >>>>> Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
