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