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 <chia7...@gmail.com> wrote: > hi Rich > > > Header.lastHeader(Iterable<Header>)? 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 > after it gets release even though I filed another KIP to add helper. > > Also, that is why I stick on the APIs. > > > KafkaProducer already setReadOnly > < > > https://github.com/apache/kafka/blob/0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L1058C13-L1058C24 > > > during `doSend`. > It is more of an implementation detail. Overall, I believe the interface of > `Headers` should be kept as is. > > What if the callback is invoked on the error path ( > > https://github.com/apache/kafka/blob/0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L1111 > )? > the simple solution is to call `setReadOnly` to make it immutable, so it is > fine. > > In fact, I'm not a fan of `Headers` as it uses "exception" to prevent the > modification. IMHO, the better design is to define a read-only interface. > At any rate, that is not related to this KIP. > > thanks for having discussion with me. I will +1 to this KIP and file a Jira > to discuss the interface changes of `Headers` > > Best, > Chia-Ping > > > > > Rich C. <chenjy.r...@gmail.com> 於 2024年8月24日 週六 上午9:30寫道： > > > 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() > > < > > > https://github.com/apache/kafka/blob/0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a/clients/src/main/java/org/apache/kafka/common/header/internals/RecordHeaders.java#L84-L94 > > > > > implementation to a static helper method like > > Header.lastHeader(Iterable<Header>)? 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. > > > > > should we pass a mutable interface to users in those callback function? > > > > This is a great question. Passing read-only Headers to these callbacks > > would maintain a unified experience and also address your concern. In > fact, > > KafkaProducer already setReadOnly > > < > > > https://github.com/apache/kafka/blob/0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L1058C13-L1058C24 > > > > > during `doSend`. > > It is more of an implementation detail. Overall, I believe the interface > of > > `Headers` should be kept as is. > > > > What do you think? > > ``` > > onAcknowledgement(RecordMetadata metadata, Exception exception, Headers > > headers) > > onCompletion(RecordMetadata metadata, Exception exception, Headers > headers) > > ``` > > > > Regards, > > Rich > > > > > > On Fri, Aug 23, 2024 at 2:12 AM Chia-Ping Tsai <chia7...@gmail.com> > wrote: > > > > > 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. <chenjy.r...@gmail.com> 於 2024年8月23日 中午12:00 寫道： > > > > > > > > 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. <chenjy.r...@gmail.com> > wrote: > > > >> > > > >> Hi Chia-Ping, > > > >> > > > >> I agree that `Headers > > > >> < > > > > > > https://kafka.apache.org/38/javadoc/org/apache/kafka/common/header/Headers.html > > > >` > > > >> extending `Iterable<Header>` includes unnecessary mutable methods > like > > > >> add() and remove(). However, Headers does offer a useful > > `lastHeader()` > > > >> method. > > > >> While I understand your point that it's not overly complex to > iterate > > > >> through and get the last header, I'm concerned that using > > > >> `Iterable<Header>` might create a *non-unified developer experience* > > > >> between these callbacks and the existing ProducerRecord.headers() > > > >> < > > > > > > https://kafka.apache.org/38/javadoc/org/apache/kafka/clients/producer/ProducerRecord.html#headers() > > > > > > > >> and ConsumerRecord.headers() > > > >> < > > > > > > https://kafka.apache.org/38/javadoc/org/apache/kafka/clients/consumer/ConsumerRecord.html#headers() > > > > > > > >> . > > > >> > > > >> Could this be an issue? > > > >> > > > >> Regards, > > > >> Rich > > > >> > > > >> > > > >> On Thu, Aug 22, 2024 at 10:05 PM Chia-Ping Tsai <chia7...@gmail.com > > > > > >> wrote: > > > >> > > > >>> hi Rich > > > >>> > > > >>> Headers is a sub class of Iterable<Header> already, so changing the > > > type > > > >>> from Headers to Iterable<Header> 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. <chenjy.r...@gmail.com> 於 2024年8月23日 上午8:26 寫道： > > > >>>> > > > >>>> Hi Chia-Ping, > > > >>>> > > > >>>> I initially considered `Iterable<Header>`, but eventually went > with > > > >>>> `Headers`. Because ProducerRecord.headers() > > > >>>> < > > > >>> > > > > > > https://kafka.apache.org/38/javadoc/org/apache/kafka/clients/producer/ProducerRecord.html#headers() > > > >>>> > > > >>>> type is `Headers`. > > > >>>> > > > >>>> Regards, > > > >>>> Rich > > > >>>> > > > >>>> > > > >>>>> On Thu, Aug 22, 2024 at 4:50 PM Chia-Ping Tsai < > > chia7...@apache.org> > > > >>> wrote: > > > >>>>> > > > >>>>> hi Rich > > > >>>>> > > > >>>>> Sorry for late response. I have just one comment: > > > >>>>> > > > >>>>> Have we consider replacing `Headers` by `Iterable<Header>`? 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 > > > >>> results > > > >>>>> as `Headers` can be either readonly of modifiable. > > > >>>>> > > > >>>>> Best, > > > >>>>> Chia-Ping > > > >>>>> > > > >>>>>> On 2024/07/23 03:13:59 "Rich C." 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 > > > >>>>>> > > > >>>>> > > > >>> > > > >> > > > > > >
