Hey contributors, Please vote or veto the proposal.
Please don't ghost. Humble request. Thanks On Thu, Sep 28, 2023 at 7:23 AM Jack Tomy <jacktomy...@gmail.com> wrote: > Bumping up..... > > On Sun, Sep 17, 2023 at 9:34 AM Jack Tomy <jacktomy...@gmail.com> wrote: > >> Hey Ismael, Sagar and everyone, >> >> Sorry I seem to have interpreted the thread wrong. Before we go ahead >> with the DTO based approach I have few reasons not to go with it. >> a. It is not following the pattern we are following today. But here I >> agree that patterns are to be changed for good. >> b. The client is not supposed to modify the record object (This is my >> understanding, If this is not a necessary requirement, please call it >> out.), passing the entire object lets the client do that. To avoid that, >> there has to be a way to deep copy the record object each time, this adds >> unnecessary requirements on the record object to support the deepcopy >> implementation. I see a lot of complexity and coupling coming in here due >> to this N I believe it's a strong reason not to go ahead with the DTO >> approach. >> >> Please let me know what you think. >> >> Thanks. >> >> >> >> >> >> On Wed, Sep 6, 2023 at 7:06 AM Sagar <sagarmeansoc...@gmail.com> wrote: >> >>> 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* >>> > >>> >> >> >> -- >> Best Regards >> *Jack* >> > > > -- > Best Regards > *Jack* > -- Best Regards *Jack*