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