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*
