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*
