Hi Rich,
Yes, you’re quite right. I realised after I’d sent it and was just about to
send a retraction.
It’s a bit odd having two defaults, but it’s necessary here because we
want the implementor to have the choice of which method to implement.
I’m happy with the KIP now.
Thanks,
Andrew
> On 10 Aug 2024, at 04:40, Rich C. <chenjy.r...@gmail.com> wrote:
>
> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>
>>
