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 <chia7...@gmail.com> wrote: > > I lean a > little bit towards keeping this KIP close to its initial motivation > (onAck). We can tackle/discuss the onCompletion separately when needed (is > there a motivation to include headers on the onCompletation that would > justify these new changes we're seeing now?). > > sounds good to me :) > > Lianet M. <liane...@gmail.com> 於 2024年9月6日 週五 上午2:28寫道： > > > Hey here, > > > > Agree we shouldn't go on with that breaking change due to the > > onCompletion+header on Callback. The onCompletion was only a "nice to > have" > > that we ended up with while discussing the onAck (motivation for this > KIP). > > The suggested alternative with the new SendCallback makes total sense to > > me, and I would be ok to review if we decide to go for it, but I lean a > > little bit towards keeping this KIP close to its initial motivation > > (onAck). We can tackle/discuss the onCompletion separately when needed > (is > > there a motivation to include headers on the onCompletation that would > > justify these new changes we're seeing now?). > > > > Thanks! > > > > Lianet > > > > On Thu, Sep 5, 2024 at 12:46 PM Matthias J. Sax <mj...@apache.org> > wrote: > > > > > Thanks for raising this issue. I agree that we cannot apply such a > > > breaking change. > > > > > > Guess it's your call if you just want to exclude `Callback` from the > KIP > > > of follow up with Chia-Ping's idea. I am happy both ways. > > > > > > > > > -Matthias > > > > > > On 9/5/24 00:00, Chia-Ping Tsai wrote: > > > > hi Rich, > > > > > > > > another way is to create a new functional interface "SendCallback", > and > > > add new `Producer.send(ProducerRecord, SendCallback)` > > > > > > > > That means both `Callback` and `Producer.send(ProducerRecord, > > Callback)` > > > needs to be deprecated. > > > > > > > > Best, > > > > Chia-Ping > > > > > > > > > > > > On 2024/09/05 01:36:25 "Rich C." wrote: > > > >> Hi all, > > > >> > > > >> KIP-512 had been accepted and I started implementing it. > > > >> `onAcknowledgement` is trivial and I am waiting for CI to pass > before > > I > > > >> mark it ready to review: https://github.com/apache/kafka/pull/17099 > > > >> > > > >> However, `onCompletion` will be a breaking change. Even though Java > > > >> supports `default` interface, it is tricky when the interface > contains > > > only > > > >> 1 abstract method. Errors are following and detail can be found at > > > >> > https://github.com/Shopify/apache-kafka/pull/2#discussion_r1744650276 > > . > > > >> > > > >> ``` > > > >> > > > > > > /home/jenkins/jenkins-agent/workspace/Kafka_kafka-pr_PR-17079/trogdor/src/main/java/org/apache/kafka/trogdor/workload/RoundTripWorker.java:259: > > > >> error: incompatible types: Callback is not a functional interface > > > >> producer.send(record, (metadata, exception) -> > { > > > >> ^ > > > >> no abstract method found in interface Callback > > > >> ``` > > > >> > > > >> I agree extending to `onCompletion` is a bonus, but from the above > > > breaking > > > >> change, do you think it is still worth it? > > > >> > > > >> (cc Andrew and Matthias since you both mention `onCompletion`) > > > >> > > > >> Regards, > > > >> Rich > > > >> > > > >> > > > >> On Sat, Aug 24, 2024 at 8:41 PM Rich C. <chenjy.r...@gmail.com> > > wrote: > > > >> > > > >>> 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 > > > >>>>>>>>>>>> > > > >>>>>>>>>>> > > > >>>>>>>>> > > > >>>>>>>> > > > >>>>>> > > > >>>>> > > > >>>> > > > >>> > > > >> > > > > > >
