Hey Jack, The way I interpreted this thread, it seems like there's more alignment on the DTO based approach. I spent some time on the suggestion that Ismael had regarding the usage of ProducerRecord. Did you get a chance to look at the reply I had posted and whether that makes sense? Also, checking out the AdminClient APIs examples provided by Ismael will give you more context. Let me know what you think.
Thanks! Sagar. On Thu, Aug 31, 2023 at 12:49 PM Jack Tomy <jacktomy...@gmail.com> wrote: > Hey everyone, > > As I see devs favouring the current style of implementation, and that is > inline with existing code. I would like to go ahead with the same approach > as mentioned in the KIP. > Can I get a few more votes so that I can take the KIP forward. > > Thanks > > > > On Sun, Aug 27, 2023 at 1:38 PM Sagar <sagarmeansoc...@gmail.com> wrote: > > > Hi Ismael, > > > > Thanks for pointing us towards the direction of a DTO based approach. The > > AdminClient examples seem very neat and extensible in that sense. > > Personally, I was trying to think only along the lines of how the current > > Partitioner interface has been designed, i.e having all requisite > > parameters as separate arguments (Topic, Key, Value etc). > > > > Regarding this question of yours: > > > > A more concrete question: did we consider having the method `partition` > > > take `ProduceRecord` as one of its parameters and `Cluster` as the > other? > > > > > > No, I don't think in the discussion thread it was brought up and as I > said > > it appears that could be due to an attempt to keep the new method's > > signature similar to the existing one within Partitioner. If I understood > > the intent of the question correctly, are you trying to hint here that > > `ProducerRecord` already contains all the arguments that the `partition` > > method accepts and also has a `headers` field within it. So, instead of > > adding another method for the `headers` field, why not create a new > method > > taking ProducerRecord directly? > > > > If my understanding is correct, then it seems like a very clean way of > > adding support for `headers`. Anyways, the partition method within > > KafkaProducer already takes a ProducerRecord as an argument so that makes > > it easier. Keeping that in mind, should this new method's (which takes a > > ProducerRecord as an input) default implementation invoke the existing > > method ? One challenge I see there is that the existing partition method > > expects serialized keys and values while ProducerRecord doesn't have > access > > to those (It directly operates on K, V). > > > > Thanks! > > Sagar. > > > > > > On Sun, Aug 27, 2023 at 8:51 AM Ismael Juma <m...@ismaeljuma.com> wrote: > > > > > A more concrete question: did we consider having the method `partition` > > > take `ProduceRecord` as one of its parameters and `Cluster` as the > other? > > > > > > Ismael > > > > > > On Sat, Aug 26, 2023 at 12:50 PM Greg Harris > > <greg.har...@aiven.io.invalid > > > > > > > wrote: > > > > > > > Hey Ismael, > > > > > > > > > The mention of "runtime" is specific to Connect. When it comes to > > > > clients, > > > > one typically compiles and runs with the same version or runs with a > > > newer > > > > version than the one used for compilation. This is standard practice > in > > > > Java and not something specific to Kafka. > > > > > > > > When I said "older runtimes" I was being lazy, and should have said > > > > "older versions of clients at runtime," thank you for figuring out > > > > what I meant. > > > > > > > > I don't know how common it is to compile a partitioner against one > > > > version of clients, and then distribute and run the partitioner with > > > > older versions of clients and expect graceful degradation of > features. > > > > If you say that it is very uncommon and not something that we should > > > > optimize for, then I won't suggest otherwise. > > > > > > > > > With regards to the Admin APIs, they have been extended several > times > > > > since introduction (naturally). One of them is: > > > > > > > > > > > > > > > https://github.com/apache/kafka/commit/1d22b0d70686aef5689b775ea2ea7610a37f3e8c > > > > > > > > Thanks for the example. I also see that includes a migration from > > > > regular arguments to the DTO style, consistent with your > > > > recommendation here. > > > > > > > > I think the DTO style and the proposed additional argument style are > > > > both reasonable. > > > > > > > > Thanks, > > > > Greg > > > > > > > > On Sat, Aug 26, 2023 at 9:46 AM Ismael Juma <m...@ismaeljuma.com> > wrote: > > > > > > > > > > Hi Greg, > > > > > > > > > > The mention of "runtime" is specific to Connect. When it comes to > > > > clients, > > > > > one typically compiles and runs with the same version or runs with > a > > > > newer > > > > > version than the one used for compilation. This is standard > practice > > in > > > > > Java and not something specific to Kafka. > > > > > > > > > > With regards to the Admin APIs, they have been extended several > times > > > > since > > > > > introduction (naturally). One of them is: > > > > > > > > > > > > > > > > > > > > https://github.com/apache/kafka/commit/1d22b0d70686aef5689b775ea2ea7610a37f3e8c > > > > > > > > > > Ismael > > > > > > > > > > On Sat, Aug 26, 2023 at 8:29 AM Greg Harris > > > <greg.har...@aiven.io.invalid > > > > > > > > > > wrote: > > > > > > > > > > > Hey Ismael, > > > > > > > > > > > > Thank you for clarifying where the DTO pattern is used already, I > > did > > > > > > not have the admin methods in mind. > > > > > > > > > > > > > With the DTO approach, you don't create a new DTO, you simply > > add a > > > > new > > > > > > overloaded constructor and accessor to the DTO. > > > > > > > > > > > > With this variant, partitioner implementations would receive a > > > > > > `NoSuchMethodException` when trying to access newer methods in > > older > > > > > > runtimes. Do we expect the interface implementers will maintain > the > > > > > > try-catch to support backwards-compatibility? > > > > > > Fortunately here the Headers type already exists, but in the > future > > > if > > > > > > a new subtype is added at the same time as the change to the DTO > is > > > > > > made, interface implementers will need to be careful to avoid > > > > > > NoClassDefFoundErrors. > > > > > > We used this "add a new method" style extension in > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-610%3A+Error+Reporting+in+Sink+Connectors > > > > > > and had to be very specific in recommending how users interact > with > > > > > > the new extension point, and there ended up being lots of sharp > > edges > > > > > > in practice. > > > > > > > > > > > > Do you have any examples of a DTO-based API that has been > extended > > > > > > since it was initially implemented? I'm not familiar with the > > > > > > evolution of the Admin APIs. > > > > > > > > > > > > Thanks! > > > > > > Greg > > > > > > > > > > > > On Sat, Aug 26, 2023 at 6:45 AM Ismael Juma <m...@ismaeljuma.com> > > > wrote: > > > > > > > > > > > > > > Hi Greg, > > > > > > > > > > > > > > The point is that the approach proposed here introduces > > complexity > > > > > > forever. > > > > > > > Each new user of this interface that needs access to the > > parameters > > > > not > > > > > > > exposed originally needs to implement the abstract method with > an > > > > empty > > > > > > > implementation and it needs to override whichever additional > > > default > > > > they > > > > > > > care about (this KIP introduces a second method, but future > KIPs > > > > would > > > > > > > introduce additional methods for new parameters). One would > never > > > > design > > > > > > > the interface like this from the start. > > > > > > > > > > > > > > With the DTO approach, you don't create a new DTO, you simply > > add a > > > > new > > > > > > > overloaded constructor and accessor to the DTO. The > implementers > > of > > > > the > > > > > > > interface still have a single method (two here since we made a > > > > mistake > > > > > > > originally) and they can decide which of the values from the > DTO > > > they > > > > > > would > > > > > > > like to access. This approach has been the recommended approach > > for > > > > years > > > > > > > and it's how the Admin apis work (they're the most recent > > client). > > > An > > > > > > > example: > > > > > > > > > > > > > > createTopics(Collection<NewTopic> newTopics, > CreateTopicsOptions > > > > > > options); > > > > > > > > > > > > > > This makes it easy to add new fields to `NewTopic` or > > > > > > `CreateTopicsOptions`. > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > On Wed, Aug 23, 2023 at 11:48 AM Greg Harris > > > > > > <greg.har...@aiven.io.invalid> > > > > > > > wrote: > > > > > > > > > > > > > > > Hey Jack, > > > > > > > > > > > > > > > > The design of this KIP is also consistent with the way header > > > > support > > > > > > > > was added to Connect: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-440%3A+Extend+Connect+Converter+to+support+headers > > > > > > > > I think making argument for precedent here is reasonable. > > > > > > > > > > > > > > > > Hi Ismael, > > > > > > > > > > > > > > > > Can you expand what you mean by "without breaking > > > compatibility"? I > > > > > > > > think the approach proposed here (a default method) would be > > > > backwards > > > > > > > > compatible. If an implementation wishes to make use of the > new > > > > > > > > signature, they can override the new method and the version > of > > > > Kafka > > > > > > > > will determine which implementation is used without instance > > > > checking, > > > > > > > > reflection, or exceptions. > > > > > > > > > > > > > > > > I believe that when you pass a DTO, that some sort of > instance > > > > > > > > checking, reflection, or exceptions would be required for the > > > > > > > > Partitioner to determine whether additional information is > > > present. > > > > > > > > For example, if we wished to add some information X to the > > > > partitioner > > > > > > > > in the future, the caller could pass either a `PartitionInfo` > > or > > > > > > > > `PartitionInfoWithX` DTO instance, and the callee could use > an > > > > > > > > `instanceof` check and a cast before accessing the X > > information. > > > > That > > > > > > > > seems to be more machinery for the Partitioner implementation > > to > > > > > > > > manage as compared to maintaining multiple methods, which may > > > just > > > > be > > > > > > > > one-line calls to other methods. > > > > > > > > > > > > > > > > Please let me know if I've misunderstood your DTO design. > > > > > > > > > > > > > > > > Thanks! > > > > > > > > Greg > > > > > > > > > > > > > > > > On Tue, Aug 22, 2023 at 9:33 PM Jack Tomy < > > jacktomy...@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi Ismael, > > > > > > > > > > > > > > > > > > That would be totally different from the pattern currently > > > being > > > > > > followed > > > > > > > > > in all the interfaces, for example serializer. > > > > > > > > > I personally don't favour that either. Let's see if the > > > community > > > > > > has any > > > > > > > > > opinions on the same. > > > > > > > > > > > > > > > > > > Hey everyone, please share your thoughts on using a DTO > > instead > > > > of > > > > > > > > separate > > > > > > > > > params for the interface. > > > > > > > > > > > > > > > > > > Thanks. > > > > > > > > > > > > > > > > > > On Mon, Aug 21, 2023 at 8:06 PM Ismael Juma < > > m...@ismaeljuma.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Jack, > > > > > > > > > > > > > > > > > > > > I mean a DTO. That means you can add additional > parameters > > > > later > > > > > > > > without > > > > > > > > > > breaking compatibility. The current proposal would result > > in > > > > yet > > > > > > > > another > > > > > > > > > > method each time we need to add parameters. > > > > > > > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > > > > > On Sun, Aug 20, 2023 at 4:53 AM Jack Tomy < > > > > jacktomy...@gmail.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > Hey Ismael, > > > > > > > > > > > > > > > > > > > > > > Are you suggesting to pass a param like a DTO or you > are > > > > > > suggesting > > > > > > > > to > > > > > > > > > > pass > > > > > > > > > > > the record object? > > > > > > > > > > > > > > > > > > > > > > I would also like to hear other devs' opinions on this > > as I > > > > > > > > personally > > > > > > > > > > > favour what is done currently. > > > > > > > > > > > > > > > > > > > > > > On Thu, Aug 17, 2023 at 9:34 AM Ismael Juma < > > > > m...@ismaeljuma.com> > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > Hi, > > > > > > > > > > > > > > > > > > > > > > > > Thanks for the KIP. The problem outlined here is a > > great > > > > > > example > > > > > > > > why we > > > > > > > > > > > > should be using a record-like structure to pass the > > > > parameters > > > > > > to a > > > > > > > > > > > method > > > > > > > > > > > > like this. Then we can add more parameters without > > having > > > > to > > > > > > > > introduce > > > > > > > > > > > new > > > > > > > > > > > > methods. Have we considered this option? > > > > > > > > > > > > > > > > > > > > > > > > Ismael > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Aug 7, 2023 at 5:26 AM Jack Tomy < > > > > > > jacktomy...@gmail.com> > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > > > > Hey everyone. > > > > > > > > > > > > > > > > > > > > > > > > > > I would like to call for a vote on KIP-953: > partition > > > > method > > > > > > to > > > > > > > > be > > > > > > > > > > > > > overloaded to accept headers as well. > > > > > > > > > > > > > > > > > > > > > > > > > > KIP : > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937 > > > > > > > > > > > > > Discussion thread : > > > > > > > > > > > > > > > > > > > https://lists.apache.org/thread/0f20kvfqkmhdqrwcb8vqgqn80szcrcdd > > > > > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > -- > > > > > > > > > > > > > Best Regards > > > > > > > > > > > > > *Jack* > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > > > Best Regards > > > > > > > > > > > *Jack* > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Best Regards > > > > > > > > > *Jack* > > > > > > > > > > > > > > > > > > > > > > > > > > -- > Best Regards > *Jack* >
