Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
A last reminder :( On Sun, Oct 15, 2023 at 1:25 PM Jack Tomy 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 wrote: > >> Bumping up. >> >> On Sun, Sep 17, 2023 at 9:34 AM Jack Tomy 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 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 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 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 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 > > >>> > > > > > > > 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
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 wrote: > Bumping up. > > On Sun, Sep 17, 2023 at 9:34 AM Jack Tomy 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 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 >>> 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 >>> 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 >>> 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 >>> > > >> > > > > >>> > > > 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 >>> > > >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
Bumping up. On Sun, Sep 17, 2023 at 9:34 AM Jack Tomy 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 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 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 >> 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 >> 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 >> > > > > > > > >> > > > 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
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 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 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 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 > > > > > > > > > > > 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
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 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 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 > > > > > > > > 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: > > > > > >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 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 > > > > > 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
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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 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 > > 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
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
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* >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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*
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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*
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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* >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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*
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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* >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
+1 (non-binding) Thanks for your KIP! From: dev@kafka.apache.org At: 08/16/23 04:48:13 UTC-4:00To: dev@kafka.apache.org Subject: Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well. Thanks Sagar and Chris for your votes. I will add the details Chris has asked for to the KIP. Hey everyone, Please consider this as a gentle reminder. On Sat, Aug 12, 2023 at 10:48 AM Chris Egerton wrote: > Hi Jack, > > +1 (binding) > > Some friendly, non-blocking suggestions: > > - IMO it's still worth specifying that the headers will be read-only; this > clarifies the intended API contract both for reviewers of the KIP who > haven't read the GitHub PR yet, and for developers who may leverage this > new method > - May be worth mentioning in the compatibility section that any > partitioners that only implement the new interface will be incompatible > with older Kafka clients versions (this is less likely to be a serious > problem in the clients world, but it's a much hairier problem with Connect, > where cross-compatibility between newer/older versions of connectors and > the Kafka Connect runtime is a serious concern) > > Again, these are not blockers and I'm in favor of the KIP with or without > them since I believe both can be addressed at least partially during PR > review and don't have to be tackled at this stage. > > Cheers, > > Chris > > On Sat, Aug 12, 2023 at 12:43 AM Sagar wrote: > > > Hey jack , > > > > +1 (non binding) > > > > Sagar. > > > > On Sat, 12 Aug 2023 at 8:04 AM, Jack Tomy wrote: > > > > > Hey everyone, > > > > > > Please consider this as a gentle reminder. > > > > > > On Mon, Aug 7, 2023 at 5:55 PM 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*
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
Thanks Sagar and Chris for your votes. I will add the details Chris has asked for to the KIP. Hey everyone, Please consider this as a gentle reminder. On Sat, Aug 12, 2023 at 10:48 AM Chris Egerton wrote: > Hi Jack, > > +1 (binding) > > Some friendly, non-blocking suggestions: > > - IMO it's still worth specifying that the headers will be read-only; this > clarifies the intended API contract both for reviewers of the KIP who > haven't read the GitHub PR yet, and for developers who may leverage this > new method > - May be worth mentioning in the compatibility section that any > partitioners that only implement the new interface will be incompatible > with older Kafka clients versions (this is less likely to be a serious > problem in the clients world, but it's a much hairier problem with Connect, > where cross-compatibility between newer/older versions of connectors and > the Kafka Connect runtime is a serious concern) > > Again, these are not blockers and I'm in favor of the KIP with or without > them since I believe both can be addressed at least partially during PR > review and don't have to be tackled at this stage. > > Cheers, > > Chris > > On Sat, Aug 12, 2023 at 12:43 AM Sagar wrote: > > > Hey jack , > > > > +1 (non binding) > > > > Sagar. > > > > On Sat, 12 Aug 2023 at 8:04 AM, Jack Tomy wrote: > > > > > Hey everyone, > > > > > > Please consider this as a gentle reminder. > > > > > > On Mon, Aug 7, 2023 at 5:55 PM 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*
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
Hi Jack, +1 (binding) Some friendly, non-blocking suggestions: - IMO it's still worth specifying that the headers will be read-only; this clarifies the intended API contract both for reviewers of the KIP who haven't read the GitHub PR yet, and for developers who may leverage this new method - May be worth mentioning in the compatibility section that any partitioners that only implement the new interface will be incompatible with older Kafka clients versions (this is less likely to be a serious problem in the clients world, but it's a much hairier problem with Connect, where cross-compatibility between newer/older versions of connectors and the Kafka Connect runtime is a serious concern) Again, these are not blockers and I'm in favor of the KIP with or without them since I believe both can be addressed at least partially during PR review and don't have to be tackled at this stage. Cheers, Chris On Sat, Aug 12, 2023 at 12:43 AM Sagar wrote: > Hey jack , > > +1 (non binding) > > Sagar. > > On Sat, 12 Aug 2023 at 8:04 AM, Jack Tomy wrote: > > > Hey everyone, > > > > Please consider this as a gentle reminder. > > > > On Mon, Aug 7, 2023 at 5:55 PM 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* > > >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
Hey jack , +1 (non binding) Sagar. On Sat, 12 Aug 2023 at 8:04 AM, Jack Tomy wrote: > Hey everyone, > > Please consider this as a gentle reminder. > > On Mon, Aug 7, 2023 at 5:55 PM 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* >
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
Hey everyone, Please consider this as a gentle reminder. On Mon, Aug 7, 2023 at 5:55 PM 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*
[VOTE] KIP-953: partition method to be overloaded to accept headers as well.
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*
