Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 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 > > > 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 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 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 > > > > > > > wrote: > > > > > > > > > Hey Jack, > > > > > > > > >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 > 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 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 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 > > > > > 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)
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 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 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 > > > 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 > 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
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 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 > 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 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 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 > > 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
[DISCUSS] KIP-971 Expose replication-offset-lag MirrorMaker2 metric
Relatively minor change with a new metric for MM2 https://cwiki.apache.org/confluence/display/KAFKA/KIP-971%3A+Expose+replication-offset-lag+MirrorMaker2+metric
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 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 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 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 > 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 > 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 > > > 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* >
