Hi all,
Can this KIP get to voting? I don't see any major concerns so far. It is
already adding ProducerRecord in Producer Interceptor which was one of the
main ask.
Thanks
Maulin
On Mon, Sep 16, 2019 at 4:09 PM Renuka M <renumetuk...@gmail.com> wrote:
> Hi All,
>
> The motivation behind this KIP is have info about record in
> ProducerInterceptor.onAcknowledgement(..), so that we can link record to
> its metadata which is missing now. Initially we proposed to add headers to
> RecordMetadata, so that we can link record to its metadata.
> After thinking little bit more about it, we thought adding a record itself
> to ProducerInterceptor.onAcknowledgement(..) makes sense since we can have
> all data to link record to its metadata in Interceptors.
>
> *public interface ProducerInterceptor<K, V> extends Configurable *{
> ....
>
>
>
>
>
> *@Deprecated public void onAcknowledgement(RecordMetadata metadata,
> Exception exception); public default void
> onAcknowledgement(RecordMetadata metadata, Exception exception,
> ProducerRecord<K, V> record){ onAcknowledgement(metadata, exception);
> }*
> }
>
> we will not be adding new method to Callback which has same method
> signature, since in callback the record information is already available
> to link.
>
>
> Please let us know your thoughts on this? If its makes sense, we will drop
> original plan of adding headers to metadata, instead we can add an
> overloaded onAcknowledgement(...) method to ProducerInterceptor with record
> itself.
> By providing above default implementation, we will have backward
> compatibility, at the same time we can have deprecation plan for existing
> method.
>
>
> Thanks
> Renuka M
>
>
> On Thu, Sep 12, 2019 at 12:22 PM Renuka M <renumetuk...@gmail.com> wrote:
>
> > Bumping this in the hope I can get more feedback and/or votes.
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-512%3AAdding+headers+to+RecordMetaData
> >
> > Thanks,
> > Renuka M
> >
> > On Mon, Sep 9, 2019 at 2:41 PM Renuka M <renumetuk...@gmail.com> wrote:
> >
> >>
> >> Hi All,
> >>
> >> Could you please take a look and let us know what you think on KIP-512
> --
> >> adding record headers to RecordMetaData.
> >> Since headers also provides metadata about the record, adding these
> >> to RecordMetaData, will allow to link record to its acknowledgement in
> >> Interceptors.
> >>
> >> The message tracing solutions like inca -
> >>
> https://medium.com/@NetflixTechBlog/inca-message-tracing-and-loss-detection-for-streaming-data-netflix-de4836fc38c9
> >> using producer callback to link records to its acknowledgement.
> >> If we have headers info as part of RecordMetaData, we can capture the
> >> record tracking in ProducerInterceptors itself.
> >>
> >> This is what we are trying to solve.
> >>
> >> Thanks
> >> Renuka M
> >>
> >>
> >>
> >>
> >>
> >> On Thu, Aug 29, 2019 at 10:49 AM Renuka M <renumetuk...@gmail.com>
> wrote:
> >>
> >>> Hi Colin,
> >>>
> >>> yes we agree but RecordMetadata in Interceptors and callbacks will not
> >>> have headers which gives context on for which record this MetaData
> belongs
> >>> to. To fill that Gap, we are proposing these changes.
> >>>
> >>>
> >>> Thanks
> >>> Renuka M
> >>>
> >>> On Thu, Aug 29, 2019 at 10:20 AM Colin McCabe <cmcc...@apache.org>
> >>> wrote:
> >>>
> >>>> As Gwen commented earlier, the client already has the record that it
> >>>> sent, including all the headers.
> >>>>
> >>>> >
> >>>> > Future<RecordMetadata> future = producer.send(myRecord, null);
> >>>> > future.get();
> >>>> > System.out.println("I sent myRecord with headers " +
> >>>> myRecord.headers());
> >>>> >
> >>>>
> >>>> best,
> >>>> Colin
> >>>>
> >>>>
> >>>> On Tue, Aug 27, 2019, at 17:06, Renuka M wrote:
> >>>> > Hi Gwen/Team
> >>>> >
> >>>> > Can you please review the KIP. Hope we have clarified the question
> >>>> you have
> >>>> > regarding proposal.
> >>>> >
> >>>> > Thanks
> >>>> > Renuka M
> >>>> >
> >>>> > On Mon, Aug 26, 2019 at 3:35 PM Renuka M <renumetuk...@gmail.com>
> >>>> wrote:
> >>>> >
> >>>> > > Hi Eric,
> >>>> > >
> >>>> > > We thought about that but we didn't find the strong enough reason
> >>>> for
> >>>> > > having record itself in Acknowledgement.
> >>>> > > Headers are supposed to carry metadata and that is the reason
> >>>> headers are
> >>>> > > added to producer/consumer records.
> >>>> > > Also we feel having headers information in record metadata is good
> >>>> enough
> >>>> > > to bridge the gap and link the record to its metadata.
> >>>> > > Its simple change since we are not adding any new method
> signatures.
> >>>> > > Adding new method signatures requires adoption and deprecation of
> >>>> old ones
> >>>> > > to reduce duplication.
> >>>> > > If we get enough votes on adding new method signature, we are open
> >>>> to add
> >>>> > > it.
> >>>> > >
> >>>> > > Thanks
> >>>> > > Renuka M
> >>>> > >
> >>>> > > On Mon, Aug 26, 2019 at 10:54 AM Eric Azama <eazama...@gmail.com>
> >>>> wrote:
> >>>> > >
> >>>> > >> Have you considered adding a new onAcknowledgement method to the
> >>>> > >> ProducerInterceptor with the signature
> >>>> onAcknowledgement(RecordMetadata
> >>>> > >> metadata, Exception exception, ProducerRecord record)? I would
> also
> >>>> > >> consider adding this to Producer Callbacks as well, since
> linking a
> >>>> > >> Callback to a specific record currently requires creating a new
> >>>> Callback
> >>>> > >> for every ProducerRecord sent.
> >>>> > >>
> >>>> > >> This seems like a more robust strategy compared to using Headers.
> >>>> Headers
> >>>> > >> don't necessarily contain anything that connects them to the
> >>>> original
> >>>> > >> ProducerRecord, and forcibly including information in the Headers
> >>>> seems
> >>>> > >> like unnecessary bloat. If your goal is to link a RecordMetadata
> >>>> to a
> >>>> > >> specific ProducerRecord, it seems simpler to make sure the
> original
> >>>> > >> ProducerRecord is accessible at the same time as the
> RecordMetadata
> >>>> > >>
> >>>> > >> On Mon, Aug 26, 2019 at 10:26 AM Renuka M <
> renumetuk...@gmail.com>
> >>>> wrote:
> >>>> > >>
> >>>> > >> > Hi Gwen,
> >>>> > >> >
> >>>> > >> > 1.We are not doing any changes on the broker side. This change
> >>>> is only
> >>>> > >> on
> >>>> > >> > Kafka clients library.
> >>>> > >> > 2. RecordMetaData is created by client library while appending
> >>>> record to
> >>>> > >> > ProducerBatch where offset alone returned by broker. Here we
> are
> >>>> adding
> >>>> > >> > headers to RecordMetaData while creating FutureRecordMetaData
> to
> >>>> create
> >>>> > >> > context between record and its metadata. I have updated the
> >>>> snippet in
> >>>> > >> KIP
> >>>> > >> > proposed changes in step 3.
> >>>> > >> > 3. As we mentioned in alternatives, client side we can link
> >>>> record and
> >>>> > >> its
> >>>> > >> > metadata using callback, but Interceptors having same
> >>>> RecordMetadata
> >>>> > >> will
> >>>> > >> > not have context on for which record this MetaData belongs to.
> >>>> To fill
> >>>> > >> that
> >>>> > >> > Gap, we are proposing these changes.
> >>>> > >> > Please let us know if we are not clear.
> >>>> > >> >
> >>>> > >> > Thanks
> >>>> > >> > Renuka M
> >>>> > >> >
> >>>> > >> >
> >>>> > >> >
> >>>> > >> >
> >>>> > >> > On Fri, Aug 23, 2019 at 7:08 PM Gwen Shapira <
> g...@confluent.io>
> >>>> wrote:
> >>>> > >> >
> >>>> > >> > > I am afraid I don't understand the proposal. The
> >>>> RecordMetadata is
> >>>> > >> > > information returned from the broker regarding the record.
> The
> >>>> > >> > > producer already has the record (including the headers), so
> >>>> why would
> >>>> > >> > > the broker need to send the headers back as part of the
> >>>> metadata?
> >>>> > >> > >
> >>>> > >> > > On Fri, Aug 23, 2019 at 4:22 PM Renuka M <
> >>>> renumetuk...@gmail.com>
> >>>> > >> wrote:
> >>>> > >> > > >
> >>>> > >> > > > Hi All,
> >>>> > >> > > >
> >>>> > >> > > > I am starting this thread to discuss
> >>>> > >> > > >
> >>>> > >> > >
> >>>> > >> >
> >>>> > >>
> >>>>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-512%3AAdding+headers+to+RecordMetaData
> >>>> > >> > > > .
> >>>> > >> > > >
> >>>> > >> > > > Please provide the feedback.
> >>>> > >> > > >
> >>>> > >> > > > Thanks
> >>>> > >> > > > Renuka M
> >>>> > >> > >
> >>>> > >> > >
> >>>> > >> > >
> >>>> > >> > > --
> >>>> > >> > > Gwen Shapira
> >>>> > >> > > Product Manager | Confluent
> >>>> > >> > > 650.450.2760 | @gwenshap
> >>>> > >> > > Follow us: Twitter | blog
> >>>> > >> > >
> >>>> > >> >
> >>>> > >>
> >>>> > >
> >>>> >
> >>>>
> >>>
>
