A last reminder :( On Sun, Oct 15, 2023 at 1:25 PM Jack Tomy <jacktomy...@gmail.com> wrote:
> 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* > -- Best Regards *Jack*