Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Thanks all. I haven't seen a strong use case that requires change on onCompletion. So I will remove it from KIP-512. When there is good motivation in the future, we can re-initialize a discussion thread and KIP. Regards, Rich On Thu, Sep 5, 2024 at 7:27 PM Chia-Ping Tsai wrote: > > I lean a >

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi Rich, Thanks for the explanation. It’s true that it breaks use of a lambda as an implementation of Callback and I would say that it’s not acceptable to do this. I don’t think I’ve seen a use of this interface which didn’t use a lambda. I believe the main focus of this KIP was the improvement

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi Chia-Ping, Thank you for the discussion and the vote. Regards, Rich On Fri, Aug 23, 2024 at 10:32 PM Chia-Ping Tsai wrote: > hi Rich > > > Header.lastHeader(Iterable)? That’s an interesting idea, but I > think it falls outside the scope of this KIP. If you’re interested, perhaps > you coul

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

hi Rich > Header.lastHeader(Iterable)? That’s an interesting idea, but I think it falls outside the scope of this KIP. If you’re interested, perhaps you could start another discussion thread or raise a separate KIP for it. The APIs included by this KIP is public, and so we can't change it easily

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi Chia-Ping, > be we can add public helpers to Header to offer “last header”? The helper can cast the Iterable to List (if ok) to get last header effectively Are you referring to extracting the RecordHeaders#lastHeader()

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

hi Rich, Thanks for sharing the case. Maybe we can add public helpers to Header to offer “last header”? The helper can cast the Iterable to List (if ok) to get last header effectively Thanks, Chia-Ping > > Rich C. 於 2024年8月23日 中午12:00 寫道： > > Hi Chia-Ping, > > To address your concern, ho

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi Chia-Ping, To address your concern, how about we clone the headers as RecordHeaders and then use setReadOnly? This approach will pass immitable interface to callback and won’t affect the developer experience. Regards, Rich On Thu, Aug 22, 2024 at 23:29 Rich C. wrote: > Hi Chia-Ping, > > I

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi Chia-Ping, I agree that `Headers ` extending `Iterable` includes unnecessary mutable methods like add() and remove(). However, Headers does offer a useful `lastHeader()` method. While I understand your point that i

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

hi Rich Headers is a sub class of Iterable already, so changing the type from Headers to Iterable does not make code complicated :) The point is “should we pass a mutable interface to users in those callback function?” IMHO, the answer is NO. Best, Chia-Ping > > Rich C. 於 2024年8月23日 上午8:2

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi Chia-Ping, I initially considered `Iterable`, but eventually went with `Headers`. Because ProducerRecord.headers() type is `Headers`. Regards, Rich On Thu, Aug 22, 2024 at 4:50 PM Chia-Ping

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

hi Rich Sorry for late response. I have just one comment: Have we consider replacing `Headers` by `Iterable`? There are some disadvantages of using `Headers`: 1. `Headers` have many setters and they are meaningless to users. 2. If users do want to modify `Headers`, they can get inconsistent re

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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,

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi all, thanks for all the feedback. As most prefer the new interface approach, I have updated KIP-512 to reflect that. If no additional feedback comes in the next few d

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

Hi Lianet, > LM1: Main advantage would be not having to add a header for calculating latency I see your points. I want to share our use case for latency measurements. We have a pipeline: `MySQL -> Debezium -> KafkaA -> Flink -> KafkaB`. In KIP-512, I simplified it to measure `Flink Sink -> KafkaB

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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). Header

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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 c

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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 wrote: > Hi, > Thanks for adding the detail. It seems quite straightforward

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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’

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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 b

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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,

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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 o

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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,

Re: [DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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. wrote: > Hi Everyone, > > I hope this email finds you well. > > I would

[DISCUSS] KIP-512: make Record Headers available in onAcknowledgement

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 Re