Hi Andrew,
> AS5: there should not be a default implementation with the current
signature
Thanks for pointing that out. But if the existing
`onAcknowledgement(RecordMetadata, Exception)` method doesn't have a
default implementation, users who want to use the new
`onAcknowledgement(RecordMetadata, Exception, Header)` method would need to
implement both. Doesn't that seem redundant? Am I missing something? Here's
an example.
```
public class MyProducerInterceptor implements ProducerInterceptor<Integer,
String> {
@Override
public void onAcknowledgement(RecordMetadata metadata, Exception
exception) {
// implemented because of part of interface and no default
// never got called
}
@Override
public void onAcknowledgement(RecordMetadata metadata, Exception
exception, Headers headers) {
// implement my logic here
}
}
```
Regards,
Rich
On Fri, Aug 9, 2024 at 4:16 AM Andrew Schofield <andrew_schofi...@live.com>
wrote:
> Hi Rich,
> Thanks for the updates. I’m generally happy with the KIP now with
> one outstanding comment.
>
> AS5: I think there’s a small error in the interface changes in the KIP.
> It seems to me that ProducerInterceptor should be changed to add
> the new `onAcknowledgement(RecordMetadata, Exception, Headers)`
> method, and this one should have a default implementation which calls
> the previous signature. However, there should not be a default
> implementation with the current signature.
>
> I think Callback has the same problem. Only the new method
> should have a default implementation.
>
>
> Thanks,
> Andrew
>
>
> > On 7 Aug 2024, at 18:16, Rich C. <chenjy.r...@gmail.com> wrote:
> >
> > Hi all,
> >
> > thanks for all the feedback. As most prefer the new interface approach, I
> > have updated KIP-512
> > <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-512%3A+make+Record+Headers+available+in+onAcknowledgement+and+onComplete
> >
> > to reflect that. If no additional feedback comes in the next few days, I
> > will start a voting thread.
> >
> > Regards,
> > Rich
> >
> >
> > On Wed, Jul 31, 2024 at 10:14 PM Rich C. <chenjy.r...@gmail.com> wrote:
> >
> >> Hi Matthias,
> >>
> >>> (100) To me, Headers store "application metadata" of a record, but they
> >> are not "Kafka native" record metadata
> >>
> >> fair enough.
> >>
> >>> (200) extending `onComplete()` and pass in the Headers might be a good
> >> thing to do, too
> >>
> >> I agree it’s a nice feature to have. We can increase the scope for now
> >> since option 1 already addresses it. If the community agrees on
> >> `onAcknowledgement` but not on `onCompletion`, we can adjust the scope
> >> accordingly. I apologize for using the term "edge case"; I would
> consider
> >> it a special case.
> >>
> >>> (200) [side mark] we should rather change the interceptor interface and
> >> change the `onSend()` return type to `void`
> >>
> >> This is out of scope, but personally, I think returning a new record
> gives
> >> developers flexibility. Imagine a ProducerInterceptor mutating two
> fields.
> >> Allowing the return of a new record makes it an all-or-nothing
> operation.
> >> If a developer chooses to mutate directly, the first mutation might
> apply
> >> successfully while the second raises an exception, resulting in a
> partial
> >> state success. Additionally, there are other reasons, such as thread
> safety.
> >>
> >>
> https://www.perplexity.ai/search/why-producerinterceptor-return-XoVscdipTYCtcXUgx6Fbyg
> >>
> >>> (300) there could be even other use-case which might benefit from an
> >> easy access to Headers in the callbacks
> >>
> >> I totally agree. Even for latency measurement, I just mentioned to
> Lianet
> >> that headers are more generic and helpful in other end-to-end latency
> >> scenarios.
> >>
> >>> (400) If we really go with overloading `onAcknowledgement()` (and maybe
> >> also `onCompletion()`), and wondering if we should deprecate the
> existing
> >> overloads?
> >>
> >> I prefer to keep the old interface. This decision is intended to help
> >> reduce the work required for client upgrades.
> >>
> >> Regards,
> >> Rich
> >>
> >>
> >> On Tue, Jul 30, 2024 at 9:48 PM Matthias J. Sax <mj...@apache.org>
> wrote:
> >>
> >>> Thanks for updating the KIP.
> >>>
> >>> (100) As stated previously, I personally don't think that adding
> Headers
> >>> to RecordMetadata is the right thing to do. To me, Headers store
> >>> "application metadata" of a record, but they are not "Kafka native"
> >>> record metadata (ie metadata Kafka an reason about). Headers are a
> black
> >>> box to Kafka, similar to key and value.
> >>>
> >>> The JavaDocs of `RecordMetadata` might not be helpful, but I agree with
> >>> Andrew and Lianet that its original purpose does not fit to the idea to
> >>> add Headers to it.
> >>>
> >>>
> >>>
> >>> (200) However, I don't understand the argument about `onComplete()`? Of
> >>> course, if the interceptor `onSend()` methods modified the headers,
> >>> `onComplete()` would see the modified record, no the original one. But
> >>> the same should be true for `onAcknowledgement()` (no matter if we pass
> >>> the Headers as parameter of via `RecordMetadata`), right?
> >>>
> >>> Thus, I would personally argue, that extending `onComplete()` and pass
> >>> in the Headers might be a good thing to do, too.
> >>>
> >>>
> >>>
> >>> [side remark] I personally don't agree to the comment on the linked PR
> >>> (even if this is not relevant to this KIP):
> >>>
> >>>> it is not recommended but allowable to create a new ProducerRecord in
> >>> Interceptor.
> >>>
> >>> In the end, `onSend()` has return type `ProducerRecord`, and while I
> >>> agree that it should be used with care, it seems odd to call it an
> "edge
> >>> case", and position it at something that is only "tolerated"... In the
> >>> end, if we really want to consider modifying a record `onSend()` as bad
> >>> practice, we should rather change the interceptor interface and change
> >>> the `onSend()` return type to `void`.
> >>>
> >>>
> >>>
> >>> (300) About Lianet's idea to just track "producer local timestamp" and
> >>> pass into the callback: I find it interesting, but I am not totally
> sure
> >>> if we might make it too complicated?
> >>>
> >>> In general, I believe that we should actually have a much larger change
> >>> to the Kafka message format, and always store the producer provided
> >>> "create timestamp" plus the broker side "log append timestamp". For
> this
> >>> case, `RecordMetadata` would be extended to also provide both
> >>> timestamps. Of course, changing the message format is totally
> >>> out-of-scope for this KIP, and I don't propose to tackle it with this
> >>> KIP. However, if we would want to be forward looking (and
> optimistically
> >>> assume that we might change the message format at some point in the
> >>> future accordingly), we could actually deprecate the existing
> >>> `RecordMetadata#timestamp()` method, and add `#createTimestamp()` and
> >>> `#logAppendTimestamp()` methods, and clearly document their semantics
> >>> with regard to the corresponding "CreateTime" vs "AppendTime" topic
> >>> config.
> >>>
> >>> The disadvantage I see is, that we do something that might never happen
> >>> in the Kafka message format, and that we would limit the scope of this
> >>> KIP to the "measure latency" use-case only.
> >>>
> >>> The KIP also mentioned tracing as use-case which could benefit to
> access
> >>> Headers in the callback what I find convincing. I don't see a good
> >>> reason why we would want to exclude this use-case? -- And frankly,
> there
> >>> could be even other use-case which might benefit from an easy access to
> >>> Headers in the callbacks.
> >>>
> >>>
> >>>
> >>> (400) If we really go with overloading `onAcknowledgement()` (and maybe
> >>> also `onCompletion()`), and wondering if we should deprecate the
> >>> existing overloads? Given that this idea is current a "rejected
> >>> alternative" it ok that the KIP is not specific about it, however, if
> we
> >>> would actually do it this way, we should consider it.
> >>>
> >>>
> >>>
> >>> -Matthias
> >>>
> >>>
> >>>
> >>> On 7/30/24 12:06 PM, Lianet M. wrote:
> >>>> Hello Rich, thanks for resurrecting the KIP, seems to fill a gap
> indeed.
> >>>>
> >>>> LM1. Specifically related to motivation#1. ProducerRecord already has
> a
> >>>> timestamp, passed into the RecordMetadata, that represents the
> creation
> >>>> time provided on new ProducerRecord, so couldn't we reuse it to avoid
> >>> the
> >>>> extra complexity of having to "include a timestamp in the header when
> >>> the
> >>>> message is sent" to be able to compute latency properly. The challenge
> >>> of
> >>>> course is that that timestamp may be overwritten (and this is the root
> >>>> cause of the gap), but that could be resolved just by keeping the
> >>> original
> >>>> time and making it available.
> >>>> RecordMetadata would keep a timestamp (passed from the record
> creation,
> >>>> never mutated), and the "effectiveTimestamp" (the one it currently
> has,
> >>>> updated with the broker result based on configs). Main advantage would
> >>> be
> >>>> not having to add a header for calculating latency. The user simply
> >>> creates
> >>>> the record with a timestamp (known existing concept), and we make that
> >>>> value accessible in the RecordMetadata (where it exists already at
> some
> >>>> point, but it's mutated). Thoughts?
> >>>>
> >>>> LM2. Regardless of the point above, if we think having the headers
> >>>> available on the onAcknowledgement would be helpful, I definitely see
> >>> the
> >>>> case for both alternatives (headers in RecordMetadata and as param). I
> >>>> share Andrew's feeling because Headers are indeed part of the
> >>>> ProducerRecord. But then headers will in practice simply contain info
> >>>> related to the record, so it seems sensible to expect to find headers
> in
> >>>> the RecordMetadata as you suggest, ok with me.
> >>>>
> >>>> Thanks!
> >>>>
> >>>> Lianet
> >>>>
> >>>> On Mon, Jul 29, 2024 at 9:41 PM Rich C. <chenjy.r...@gmail.com>
> wrote:
> >>>>
> >>>>> Hi Andrew,
> >>>>>
> >>>>> Thanks for the feedback. I have updated KIP-512 and addressed AS2,
> >>> AS3, and
> >>>>> AS4. For AS1, let's wait for further responses from the community.
> >>>>>
> >>>>>
> >>>>> Regards,
> >>>>> Rich
> >>>>>
> >>>>>
> >>>>> On Mon, Jul 29, 2024 at 5:59 AM Andrew Schofield <
> >>>>> andrew_schofi...@live.com>
> >>>>> wrote:
> >>>>>
> >>>>>> Hi,
> >>>>>> Thanks for adding the detail. It seems quite straightforward to
> >>>>>> implement in the producer code.
> >>>>>>
> >>>>>> AS1: Personally, and of course this is a matter of taste and just
> one
> >>>>>> opinion, I don’t like adding Headers to RecordMetadata. It seems to
> me
> >>>>>> that RecordMetadata is information about the record that’s been
> >>> produced
> >>>>>> whereas the Headers are really part of the record itself. So, I
> prefer
> >>>>> the
> >>>>>> alternative which overloads ProducerInterceptor.onAcknowledgement.
> >>>>>>
> >>>>>> AS2: ProducerBatch and FutureRecordMetadata are both internal
> classes
> >>>>>> and do not need to be documented in the KIP.
> >>>>>>
> >>>>>> AS3: This KIP is adding rather than replcaing the constructor for
> >>>>>> RecordMetadata.
> >>>>>> You should define the value for the Headers if an existing
> constructor
> >>>>>> without headers is used.
> >>>>>>
> >>>>>> AS4: You should add a method `Headers headers()` to RecordMetadata.
> >>>>>>
> >>>>>>
> >>>>>> I wonder what other community members think about whether it’s a
> good
> >>>>>> idea to extend RecordMetadata with the headers.
> >>>>>>
> >>>>>> Thanks,
> >>>>>> Andrew
> >>>>>>
> >>>>>>> On 29 Jul 2024, at 05:36, Rich C. <chenjy.r...@gmail.com> wrote:
> >>>>>>>
> >>>>>>> Hi all,
> >>>>>>>
> >>>>>>> Thank you for the positive feedback. I added proposal changes to
> >>>>> KIP-512
> >>>>>>> and included a FAQ section to address some concerns.
> >>>>>>>
> >>>>>>> Hi Andrew, yes, this KIP focuses on
> >>>>>>> `ProducerInterceptor.onAcknowledgement`. I added FAQ#3 to explain
> >>> that.
> >>>>>>>
> >>>>>>> Hi Matthias, for your question about "RecordMetadata being Kafka
> >>>>>> metadata" in
> >>>>>>> this thread
> >>>>>>> <
> >>>>>>
> >>>>>
> >>>
> https://lists.apache.org/list?dev@kafka.apache.org:lte=1M:make%20Record%20Headers%20available%20in%20onAcknowledgement
> >>>>>>> ,
> >>>>>>> I added FAQ#2 to explain that. If I have missed any documentation
> >>>>>> regarding
> >>>>>>> the design of RecordMetadata, please let me know.
> >>>>>>>
> >>>>>>> Regards,
> >>>>>>> Rich
> >>>>>>>
> >>>>>>>
> >>>>>>> On Fri, Jul 26, 2024 at 4:00 PM Andrew Schofield <
> >>>>>> andrew_schofi...@live.com>
> >>>>>>> wrote:
> >>>>>>>
> >>>>>>>> Hi Rich,
> >>>>>>>> Thanks for resurrecting this KIP. It seems like a useful idea to
> me
> >>>>> and
> >>>>>>>> I’d be interested in seeing the proposed public interfaces.
> >>>>>>>>
> >>>>>>>> I note that you specifically called out the
> >>>>>>>> ProducerInterceptor.onAcknowledgement
> >>>>>>>> method, as opposed to the producer Callback.onCompletion method.
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>> Andrew
> >>>>>>>>
> >>>>>>>>> On 26 Jul 2024, at 04:54, Rich C. <chenjy.r...@gmail.com> wrote:
> >>>>>>>>>
> >>>>>>>>> Hi Kevin,
> >>>>>>>>>
> >>>>>>>>> Thanks for your support.
> >>>>>>>>>
> >>>>>>>>> Hi Matthias,
> >>>>>>>>>
> >>>>>>>>> I apologize for the confusion. I've deleted the Public Interface
> >>>>>> sections
> >>>>>>>>> for now. I think we should focus on discussing its necessity with
> >>> the
> >>>>>>>>> community. I'll let it sit for a few more days, and if there are
> no
> >>>>>>>>> objections, I will propose changes over the weekend and share
> them
> >>>>> here
> >>>>>>>>> again.
> >>>>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> Rich
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> On Thu, Jul 25, 2024 at 5:51 PM Matthias J. Sax <
> mj...@apache.org>
> >>>>>>>> wrote:
> >>>>>>>>>
> >>>>>>>>>> Rich,
> >>>>>>>>>>
> >>>>>>>>>> thanks for resurrecting this KIP. I was not part of the original
> >>>>>>>>>> discussion back in the day, but personally agree with your
> >>>>> assessment
> >>>>>>>>>> that making headers available in the callbacks would make
> >>>>> developer's
> >>>>>>>>>> life much simpler.
> >>>>>>>>>>
> >>>>>>>>>> For the KIP itself, starting with "Public Interface" section,
> >>>>>> everything
> >>>>>>>>>> is formatted as "strike through". Can you fix this? It's
> confusing
> >>>>> as
> >>>>>>>>>> it's apparently not correctly formatted, but unclear which (if
> >>> any)
> >>>>>>>>>> parts should be formatted like this. In general, wiki pages have
> >>>>>>>>>> history, so strike-through should be used rather rarely but the
> >>> wiki
> >>>>>>>>>> page should just contain the latest proposal. (If one want to
> see
> >>>>> the
> >>>>>>>>>> history, it's there anyway).
> >>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>>> -Matthias
> >>>>>>>>>>
> >>>>>>>>>> On 7/23/24 6:36 AM, Kevin Lam wrote:
> >>>>>>>>>>> Hi,
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks for starting the discussion. Latency Measurement and
> >>> Tracing
> >>>>>>>>>>> Completeness are both good reasons to support this feature, and
> >>>>> would
> >>>>>>>> be
> >>>>>>>>>>> interested to see this move forward.
> >>>>>>>>>>>
> >>>>>>>>>>> On Mon, Jul 22, 2024 at 11:15 PM Rich C. <
> chenjy.r...@gmail.com>
> >>>>>>>> wrote:
> >>>>>>>>>>>
> >>>>>>>>>>>> Hi Everyone,
> >>>>>>>>>>>>
> >>>>>>>>>>>> I hope this email finds you well.
> >>>>>>>>>>>>
> >>>>>>>>>>>> I would like to start a discussion on KIP-512. The initial
> >>> version
> >>>>>> of
> >>>>>>>>>>>> KIP-512 was created in 2019, and I have resurrected it in 2024
> >>>>> with
> >>>>>>>> more
> >>>>>>>>>>>> details about the motivation behind it.
> >>>>>>>>>>>>
> >>>>>>>>>>>> You can view the current version of the KIP here: KIP-512:
> Make
> >>>>>> Record
> >>>>>>>>>>>> Headers Available in onAcknowledgement.
> >>>>>>>>>>>> <
> >>>>>>>>>>>>
> >>>>>>>>>>
> >>>>>>>>
> >>>>>>
> >>>>>
> >>>
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-512%3A+make+Record+Headers+available+in+onAcknowledgement
> >>>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>> Let's focus on discussing the necessity of this feature first.
> >>> If
> >>>>> we
> >>>>>>>>>> agree
> >>>>>>>>>>>> on its importance, we can then move on to discussing the
> >>> proposed
> >>>>>>>>>> changes.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Looking forward to your feedback.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Best regards,
> >>>>>>>>>>>> Rich
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
>
>
