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 of ProducerInterceptor.onAcknowledgement. Supplying the headers to a Callback lambda is quite straightforward even without the new method signature. I’m happy that the KIP is achieving its aim even if you just implement the change to ProducerInterceptor. Thanks, Andrew On 4 Sep 2024, at 18:36, Rich C. <chenjy.r...@gmail.com> 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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fkafka%2Fpull%2F17099&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941044898%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Ry5DuQrLS%2B7zk6yIupobhQ7e9O1oZ%2FMkNKGxMNt2Y4s%3D&reserved=0<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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FShopify%2Fapache-kafka%2Fpull%2F2%23discussion_r1744650276&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292<https://github.com/Shopify/apache-kafka/pull/2#discussion_r1744650276>%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941056353%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Deh7o%2F5t7lfoF5A%2BoboxfdphrwUhcowMW1JPUChI8PY%3D&reserved=0. ``` /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<mailto: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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fkafka%2Fblob%2F0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a%2Fclients%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkafka%2Fclients%2Fproducer%2FKafkaProducer.java%23L1058C13-L1058C24&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941063380%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=TRYoo7RHQJ%2B0nbljGUe6BODmmx4RgPIvxmEWITKjg94%3D&reserved=0<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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fkafka%2Fblob%2F0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a%2Fclients%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkafka%2Fclients%2Fproducer%2FKafkaProducer.java%23L1111&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941067993%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=VBBnReToYAkAKyXeYvtdPGPhPmYgHOTOhulmuFJ8Phs%3D&reserved=0<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<mailto: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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fkafka%2Fblob%2F0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a%2Fclients%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkafka%2Fcommon%2Fheader%2Finternals%2FRecordHeaders.java%23L84-L94&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941072421%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=kC5%2BctC%2Fik93I65cS%2F0gK97LNdOrWmkEgDDCI3uhGC4%3D&reserved=0<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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fapache%2Fkafka%2Fblob%2F0123bdcf06d2554ea62ba3a75af7c4b17eb4d23a%2Fclients%2Fsrc%2Fmain%2Fjava%2Forg%2Fapache%2Fkafka%2Fclients%2Fproducer%2FKafkaProducer.java%23L1058C13-L1058C24&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941076811%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=%2BvqvBg9X3MeKN1sar7GQnECz56wUzhNuuJeScjtt9Qo%3D&reserved=0<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<mailto: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<mailto: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<mailto:chenjy.r...@gmail.com>> wrote: Hi Chia-Ping, I agree that `Headers < https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fkafka.apache.org%2F38%2Fjavadoc%2Forg%2Fapache%2Fkafka%2Fcommon%2Fheader%2FHeaders.html&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941081167%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Tb6SV8p9AsZiawh0cKAjGUOSAUCEH5dVevZndzyPx0k%3D&reserved=0<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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fkafka.apache.org%2F38%2Fjavadoc%2Forg%2Fapache%2Fkafka%2Fclients%2Fproducer%2FProducerRecord.html%23headers&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941085292%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=Spcv0TvU1iqC8oj9zX9MD6P8c4tOKt6LfHA%2FU3gZOMU%3D&reserved=0()<https://kafka.apache.org/38/javadoc/org/apache/kafka/clients/producer/ProducerRecord.html#headers> and ConsumerRecord.headers() < https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fkafka.apache.org%2F38%2Fjavadoc%2Forg%2Fapache%2Fkafka%2Fclients%2Fconsumer%2FConsumerRecord.html%23headers&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941089428%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=XwJW%2BV1STuxrXQ2AOfx7yxkT44jOhv0ly2NhjON3Ez4%3D&reserved=0()<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<mailto: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<mailto: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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fkafka.apache.org%2F38%2Fjavadoc%2Forg%2Fapache%2Fkafka%2Fclients%2Fproducer%2FProducerRecord.html%23headers&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941093618%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=BoaUrwwB8F9ibMMZMJDmprYrO3JZNus8EziGAUrrjp4%3D&reserved=0()<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<mailto: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://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcwiki.apache.org%2Fconfluence%2Fdisplay%2FKAFKA%2FKIP-512%253A%2Bmake%2BRecord%2BHeaders%2Bavailable%2Bin%2BonAcknowledgement&data=05%7C02%7C%7C5a1469239a6f40fb470b08dccd4ba292%7C84df9e7fe9f640afb435aaaaaaaaaaaa%7C1%7C0%7C638610971941097757%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=PwzHHwnxCeHfAOQ%2FS6bVLtzsxHn00rcQfxbJXpXHNFg%3D&reserved=0<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
