Thanks Colin! Good call on the ApiRecordError. We could use
InvalidRecordException instead, and have the broker convert it
to ApiRecordError.
Modified signature below.
interface BrokerRecordValidator {
/**
* Validate the record for a given topic-partition.
*/
Optional<InvalidRecordException> validateRecord(TopicPartition
topicPartition, ByteBuffer key, ByteBuffer value, Header[] headers);
}
On Tue, Apr 6, 2021 at 5:09 PM Colin McCabe <cmcc...@apache.org> wrote:
> Hi Soumyajit,
>
> The difficult thing is deciding which fields to share and how to share
> them. Key and value are probably the minimum we need to make this useful.
> If we do choose to go with byte buffer, it is not necessary to also pass
> the size, since ByteBuffer maintains that internally.
>
> ApiRecordError is also an internal class, so it can't be used in a public
> API. I think most likely if we were going to do this, we would just catch
> an exception and use the exception text as the validation error.
>
> best,
> Colin
>
>
> On Tue, Apr 6, 2021, at 15:57, Soumyajit Sahu wrote:
> > Hi Tom,
> >
> > Makes sense. Thanks for the explanation. I get what Colin had meant
> earlier.
> >
> > Would a different signature for the interface work? Example below, but
> > please feel free to suggest alternatives if there are any possibilities
> of
> > such.
> >
> > If needed, then deprecating this and introducing a new signature would be
> > straight-forward as both (old and new) calls could be made serially in
> the
> > LogValidator allowing a coexistence for a transition period.
> >
> > interface BrokerRecordValidator {
> > /**
> > * Validate the record for a given topic-partition.
> > */
> > Optional<ApiRecordError> validateRecord(TopicPartition
> topicPartition,
> > int keySize, ByteBuffer key, int valueSize, ByteBuffer value, Header[]
> > headers);
> > }
> >
> >
> > On Tue, Apr 6, 2021 at 12:54 AM Tom Bentley <tbent...@redhat.com> wrote:
> >
> > > Hi Soumyajit,
> > >
> > > Although that class does indeed have public access at the Java level,
> it
> > > does so only because it needs to be used by internal Kafka code which
> lives
> > > in other packages (there isn't any more restrictive access modifier
> which
> > > would work). What the project considers public Java API is determined
> by
> > > what's included in the published Javadocs:
> > > https://kafka.apache.org/27/javadoc/index.html, which doesn't include
> the
> > > org.apache.kafka.common.record package.
> > >
> > > One of the problems with making these internal classes public is it
> ties
> > > the project into supporting them as APIs, which can make changing them
> much
> > > harder and in the long run that can slow, or even prevent, innovation
> in
> > > the rest of Kafka.
> > >
> > > Kind regards,
> > >
> > > Tom
> > >
> > >
> > >
> > > On Sun, Apr 4, 2021 at 7:31 PM Soumyajit Sahu <
> soumyajit.s...@gmail.com>
> > > wrote:
> > >
> > > > Hi Colin,
> > > > I see that both the interface "Record" and the implementation
> > > > "DefaultRecord" being used in LogValidator.java are public
> > > > interfaces/classes.
> > > >
> > > >
> > > >
> > >
> https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/record/Records.java
> > > > and
> > > >
> > > >
> > >
> https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/common/record/DefaultRecord.java
> > > >
> > > > So, it should be ok to use them. Let me know what you think.
> > > >
> > > > Thanks,
> > > > Soumyajit
> > > >
> > > >
> > > > On Fri, Apr 2, 2021 at 8:51 AM Colin McCabe <cmcc...@apache.org>
> wrote:
> > > >
> > > > > Hi Soumyajit,
> > > > >
> > > > > I believe we've had discussions about proposals similar to this
> before,
> > > > > although I'm having trouble finding one right now. The issue here
> is
> > > > that
> > > > > Record is a private class -- it is not part of any public API, and
> may
> > > > > change at any time. So we can't expose it in public APIs.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Thu, Apr 1, 2021, at 14:18, Soumyajit Sahu wrote:
> > > > > > Hello All,
> > > > > > I would like to start a discussion on the KIP-729.
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-729%3A+Custom+validation+of+records+on+the+broker+prior+to+log+append
> > > > > >
> > > > > > Thanks!
> > > > > > Soumyajit
> > > > > >
> > > > >
> > > >
> > >
> >
>
