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