You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Jack Tomy <ja...@gmail.com> on 2023/10/15 07:55:20 UTC
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 <ja...@gmail.com> wrote:
> Bumping up.....
>
> On Sun, Sep 17, 2023 at 9:34 AM Jack Tomy <ja...@gmail.com> wrote:
>
>> Hey Ismael, Sagar and everyone,
>>
>> Sorry I seem to have interpreted the thread wrong. Before we go ahead
>> with the DTO based approach I have few reasons not to go with it.
>> a. It is not following the pattern we are following today. But here I
>> agree that patterns are to be changed for good.
>> b. The client is not supposed to modify the record object (This is my
>> understanding, If this is not a necessary requirement, please call it
>> out.), passing the entire object lets the client do that. To avoid that,
>> there has to be a way to deep copy the record object each time, this adds
>> unnecessary requirements on the record object to support the deepcopy
>> implementation. I see a lot of complexity and coupling coming in here due
>> to this N I believe it's a strong reason not to go ahead with the DTO
>> approach.
>>
>> Please let me know what you think.
>>
>> Thanks.
>>
>>
>>
>>
>>
>> On Wed, Sep 6, 2023 at 7:06 AM Sagar <sa...@gmail.com> wrote:
>>
>>> Hey Jack,
>>>
>>> The way I interpreted this thread, it seems like there's more alignment
>>> on
>>> the DTO based approach. I spent some time on the suggestion that Ismael
>>> had
>>> regarding the usage of ProducerRecord. Did you get a chance to look at
>>> the
>>> reply I had posted and whether that makes sense? Also, checking out the
>>> AdminClient APIs examples provided by Ismael will give you more context.
>>> Let me know what you think.
>>>
>>> Thanks!
>>> Sagar.
>>>
>>> On Thu, Aug 31, 2023 at 12:49 PM Jack Tomy <ja...@gmail.com>
>>> wrote:
>>>
>>> > Hey everyone,
>>> >
>>> > As I see devs favouring the current style of implementation, and that
>>> is
>>> > inline with existing code. I would like to go ahead with the same
>>> approach
>>> > as mentioned in the KIP.
>>> > Can I get a few more votes so that I can take the KIP forward.
>>> >
>>> > Thanks
>>> >
>>> >
>>> >
>>> > On Sun, Aug 27, 2023 at 1:38 PM Sagar <sa...@gmail.com>
>>> wrote:
>>> >
>>> > > Hi Ismael,
>>> > >
>>> > > Thanks for pointing us towards the direction of a DTO based
>>> approach. The
>>> > > AdminClient examples seem very neat and extensible in that sense.
>>> > > Personally, I was trying to think only along the lines of how the
>>> current
>>> > > Partitioner interface has been designed, i.e having all requisite
>>> > > parameters as separate arguments (Topic, Key, Value etc).
>>> > >
>>> > > Regarding this question of yours:
>>> > >
>>> > > A more concrete question: did we consider having the method
>>> `partition`
>>> > > > take `ProduceRecord` as one of its parameters and `Cluster` as the
>>> > other?
>>> > >
>>> > >
>>> > > No, I don't think in the discussion thread it was brought up and as I
>>> > said
>>> > > it appears that could be due to an attempt to keep the new method's
>>> > > signature similar to the existing one within Partitioner. If I
>>> understood
>>> > > the intent of the question correctly, are you trying to hint here
>>> that
>>> > > `ProducerRecord` already contains all the arguments that the
>>> `partition`
>>> > > method accepts and also has a `headers` field within it. So, instead
>>> of
>>> > > adding another method for the `headers` field, why not create a new
>>> > method
>>> > > taking ProducerRecord directly?
>>> > >
>>> > > If my understanding is correct, then it seems like a very clean way
>>> of
>>> > > adding support for `headers`. Anyways, the partition method within
>>> > > KafkaProducer already takes a ProducerRecord as an argument so that
>>> makes
>>> > > it easier. Keeping that in mind, should this new method's (which
>>> takes a
>>> > > ProducerRecord as an input) default implementation invoke the
>>> existing
>>> > > method ? One challenge I see there is that the existing partition
>>> method
>>> > > expects serialized keys and values while ProducerRecord doesn't have
>>> > access
>>> > > to those (It directly operates on K, V).
>>> > >
>>> > > Thanks!
>>> > > Sagar.
>>> > >
>>> > >
>>> > > On Sun, Aug 27, 2023 at 8:51 AM Ismael Juma <me...@ismaeljuma.com>
>>> wrote:
>>> > >
>>> > > > A more concrete question: did we consider having the method
>>> `partition`
>>> > > > take `ProduceRecord` as one of its parameters and `Cluster` as the
>>> > other?
>>> > > >
>>> > > > Ismael
>>> > > >
>>> > > > On Sat, Aug 26, 2023 at 12:50 PM Greg Harris
>>> > > <greg.harris@aiven.io.invalid
>>> > > > >
>>> > > > wrote:
>>> > > >
>>> > > > > Hey Ismael,
>>> > > > >
>>> > > > > > The mention of "runtime" is specific to Connect. When it comes
>>> to
>>> > > > > clients,
>>> > > > > one typically compiles and runs with the same version or runs
>>> with a
>>> > > > newer
>>> > > > > version than the one used for compilation. This is standard
>>> practice
>>> > in
>>> > > > > Java and not something specific to Kafka.
>>> > > > >
>>> > > > > When I said "older runtimes" I was being lazy, and should have
>>> said
>>> > > > > "older versions of clients at runtime," thank you for figuring
>>> out
>>> > > > > what I meant.
>>> > > > >
>>> > > > > I don't know how common it is to compile a partitioner against
>>> one
>>> > > > > version of clients, and then distribute and run the partitioner
>>> with
>>> > > > > older versions of clients and expect graceful degradation of
>>> > features.
>>> > > > > If you say that it is very uncommon and not something that we
>>> should
>>> > > > > optimize for, then I won't suggest otherwise.
>>> > > > >
>>> > > > > > With regards to the Admin APIs, they have been extended several
>>> > times
>>> > > > > since introduction (naturally). One of them is:
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://github.com/apache/kafka/commit/1d22b0d70686aef5689b775ea2ea7610a37f3e8c
>>> > > > >
>>> > > > > Thanks for the example. I also see that includes a migration from
>>> > > > > regular arguments to the DTO style, consistent with your
>>> > > > > recommendation here.
>>> > > > >
>>> > > > > I think the DTO style and the proposed additional argument style
>>> are
>>> > > > > both reasonable.
>>> > > > >
>>> > > > > Thanks,
>>> > > > > Greg
>>> > > > >
>>> > > > > On Sat, Aug 26, 2023 at 9:46 AM Ismael Juma <me...@ismaeljuma.com>
>>> > wrote:
>>> > > > > >
>>> > > > > > Hi Greg,
>>> > > > > >
>>> > > > > > The mention of "runtime" is specific to Connect. When it comes
>>> to
>>> > > > > clients,
>>> > > > > > one typically compiles and runs with the same version or runs
>>> with
>>> > a
>>> > > > > newer
>>> > > > > > version than the one used for compilation. This is standard
>>> > practice
>>> > > in
>>> > > > > > Java and not something specific to Kafka.
>>> > > > > >
>>> > > > > > With regards to the Admin APIs, they have been extended several
>>> > times
>>> > > > > since
>>> > > > > > introduction (naturally). One of them is:
>>> > > > > >
>>> > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://github.com/apache/kafka/commit/1d22b0d70686aef5689b775ea2ea7610a37f3e8c
>>> > > > > >
>>> > > > > > Ismael
>>> > > > > >
>>> > > > > > On Sat, Aug 26, 2023 at 8:29 AM Greg Harris
>>> > > > <greg.harris@aiven.io.invalid
>>> > > > > >
>>> > > > > > wrote:
>>> > > > > >
>>> > > > > > > Hey Ismael,
>>> > > > > > >
>>> > > > > > > Thank you for clarifying where the DTO pattern is used
>>> already, I
>>> > > did
>>> > > > > > > not have the admin methods in mind.
>>> > > > > > >
>>> > > > > > > > With the DTO approach, you don't create a new DTO, you
>>> simply
>>> > > add a
>>> > > > > new
>>> > > > > > > overloaded constructor and accessor to the DTO.
>>> > > > > > >
>>> > > > > > > With this variant, partitioner implementations would receive
>>> a
>>> > > > > > > `NoSuchMethodException` when trying to access newer methods
>>> in
>>> > > older
>>> > > > > > > runtimes. Do we expect the interface implementers will
>>> maintain
>>> > the
>>> > > > > > > try-catch to support backwards-compatibility?
>>> > > > > > > Fortunately here the Headers type already exists, but in the
>>> > future
>>> > > > if
>>> > > > > > > a new subtype is added at the same time as the change to the
>>> DTO
>>> > is
>>> > > > > > > made, interface implementers will need to be careful to avoid
>>> > > > > > > NoClassDefFoundErrors.
>>> > > > > > > We used this "add a new method" style extension in
>>> > > > > > >
>>> > > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-610%3A+Error+Reporting+in+Sink+Connectors
>>> > > > > > > and had to be very specific in recommending how users
>>> interact
>>> > with
>>> > > > > > > the new extension point, and there ended up being lots of
>>> sharp
>>> > > edges
>>> > > > > > > in practice.
>>> > > > > > >
>>> > > > > > > Do you have any examples of a DTO-based API that has been
>>> > extended
>>> > > > > > > since it was initially implemented? I'm not familiar with the
>>> > > > > > > evolution of the Admin APIs.
>>> > > > > > >
>>> > > > > > > Thanks!
>>> > > > > > > Greg
>>> > > > > > >
>>> > > > > > > On Sat, Aug 26, 2023 at 6:45 AM Ismael Juma <
>>> me@ismaeljuma.com>
>>> > > > wrote:
>>> > > > > > > >
>>> > > > > > > > Hi Greg,
>>> > > > > > > >
>>> > > > > > > > The point is that the approach proposed here introduces
>>> > > complexity
>>> > > > > > > forever.
>>> > > > > > > > Each new user of this interface that needs access to the
>>> > > parameters
>>> > > > > not
>>> > > > > > > > exposed originally needs to implement the abstract method
>>> with
>>> > an
>>> > > > > empty
>>> > > > > > > > implementation and it needs to override whichever
>>> additional
>>> > > > default
>>> > > > > they
>>> > > > > > > > care about (this KIP introduces a second method, but future
>>> > KIPs
>>> > > > > would
>>> > > > > > > > introduce additional methods for new parameters). One would
>>> > never
>>> > > > > design
>>> > > > > > > > the interface like this from the start.
>>> > > > > > > >
>>> > > > > > > > With the DTO approach, you don't create a new DTO, you
>>> simply
>>> > > add a
>>> > > > > new
>>> > > > > > > > overloaded constructor and accessor to the DTO. The
>>> > implementers
>>> > > of
>>> > > > > the
>>> > > > > > > > interface still have a single method (two here since we
>>> made a
>>> > > > > mistake
>>> > > > > > > > originally) and they can decide which of the values from
>>> the
>>> > DTO
>>> > > > they
>>> > > > > > > would
>>> > > > > > > > like to access. This approach has been the recommended
>>> approach
>>> > > for
>>> > > > > years
>>> > > > > > > > and it's how the Admin apis work (they're the most recent
>>> > > client).
>>> > > > An
>>> > > > > > > > example:
>>> > > > > > > >
>>> > > > > > > > createTopics(Collection<NewTopic> newTopics,
>>> > CreateTopicsOptions
>>> > > > > > > options);
>>> > > > > > > >
>>> > > > > > > > This makes it easy to add new fields to `NewTopic` or
>>> > > > > > > `CreateTopicsOptions`.
>>> > > > > > > >
>>> > > > > > > > Ismael
>>> > > > > > > >
>>> > > > > > > > On Wed, Aug 23, 2023 at 11:48 AM Greg Harris
>>> > > > > > > <gr...@aiven.io.invalid>
>>> > > > > > > > wrote:
>>> > > > > > > >
>>> > > > > > > > > Hey Jack,
>>> > > > > > > > >
>>> > > > > > > > > The design of this KIP is also consistent with the way
>>> header
>>> > > > > support
>>> > > > > > > > > was added to Connect:
>>> > > > > > > > >
>>> > > > > > > > >
>>> > > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-440%3A+Extend+Connect+Converter+to+support+headers
>>> > > > > > > > > I think making argument for precedent here is reasonable.
>>> > > > > > > > >
>>> > > > > > > > > Hi Ismael,
>>> > > > > > > > >
>>> > > > > > > > > Can you expand what you mean by "without breaking
>>> > > > compatibility"? I
>>> > > > > > > > > think the approach proposed here (a default method)
>>> would be
>>> > > > > backwards
>>> > > > > > > > > compatible. If an implementation wishes to make use of
>>> the
>>> > new
>>> > > > > > > > > signature, they can override the new method and the
>>> version
>>> > of
>>> > > > > Kafka
>>> > > > > > > > > will determine which implementation is used without
>>> instance
>>> > > > > checking,
>>> > > > > > > > > reflection, or exceptions.
>>> > > > > > > > >
>>> > > > > > > > > I believe that when you pass a DTO, that some sort of
>>> > instance
>>> > > > > > > > > checking, reflection, or exceptions would be required
>>> for the
>>> > > > > > > > > Partitioner to determine whether additional information
>>> is
>>> > > > present.
>>> > > > > > > > > For example, if we wished to add some information X to
>>> the
>>> > > > > partitioner
>>> > > > > > > > > in the future, the caller could pass either a
>>> `PartitionInfo`
>>> > > or
>>> > > > > > > > > `PartitionInfoWithX` DTO instance, and the callee could
>>> use
>>> > an
>>> > > > > > > > > `instanceof` check and a cast before accessing the X
>>> > > information.
>>> > > > > That
>>> > > > > > > > > seems to be more machinery for the Partitioner
>>> implementation
>>> > > to
>>> > > > > > > > > manage as compared to maintaining multiple methods,
>>> which may
>>> > > > just
>>> > > > > be
>>> > > > > > > > > one-line calls to other methods.
>>> > > > > > > > >
>>> > > > > > > > > Please let me know if I've misunderstood your DTO design.
>>> > > > > > > > >
>>> > > > > > > > > Thanks!
>>> > > > > > > > > Greg
>>> > > > > > > > >
>>> > > > > > > > > On Tue, Aug 22, 2023 at 9:33 PM Jack Tomy <
>>> > > jacktomy107@gmail.com
>>> > > > >
>>> > > > > > > wrote:
>>> > > > > > > > > >
>>> > > > > > > > > > Hi Ismael,
>>> > > > > > > > > >
>>> > > > > > > > > > That would be totally different from the pattern
>>> currently
>>> > > > being
>>> > > > > > > followed
>>> > > > > > > > > > in all the interfaces, for example serializer.
>>> > > > > > > > > > I personally don't favour that either. Let's see if the
>>> > > > community
>>> > > > > > > has any
>>> > > > > > > > > > opinions on the same.
>>> > > > > > > > > >
>>> > > > > > > > > > Hey everyone, please share your thoughts on using a DTO
>>> > > instead
>>> > > > > of
>>> > > > > > > > > separate
>>> > > > > > > > > > params for the interface.
>>> > > > > > > > > >
>>> > > > > > > > > > Thanks.
>>> > > > > > > > > >
>>> > > > > > > > > > On Mon, Aug 21, 2023 at 8:06 PM Ismael Juma <
>>> > > me@ismaeljuma.com
>>> > > > >
>>> > > > > > > wrote:
>>> > > > > > > > > >
>>> > > > > > > > > > > Hi Jack,
>>> > > > > > > > > > >
>>> > > > > > > > > > > I mean a DTO. That means you can add additional
>>> > parameters
>>> > > > > later
>>> > > > > > > > > without
>>> > > > > > > > > > > breaking compatibility. The current proposal would
>>> result
>>> > > in
>>> > > > > yet
>>> > > > > > > > > another
>>> > > > > > > > > > > method each time we need to add parameters.
>>> > > > > > > > > > >
>>> > > > > > > > > > > Ismael
>>> > > > > > > > > > >
>>> > > > > > > > > > > On Sun, Aug 20, 2023 at 4:53 AM Jack Tomy <
>>> > > > > jacktomy107@gmail.com>
>>> > > > > > > > > wrote:
>>> > > > > > > > > > >
>>> > > > > > > > > > > > Hey Ismael,
>>> > > > > > > > > > > >
>>> > > > > > > > > > > > Are you suggesting to pass a param like a DTO or
>>> you
>>> > are
>>> > > > > > > suggesting
>>> > > > > > > > > to
>>> > > > > > > > > > > pass
>>> > > > > > > > > > > > the record object?
>>> > > > > > > > > > > >
>>> > > > > > > > > > > > I would also like to hear other devs' opinions on
>>> this
>>> > > as I
>>> > > > > > > > > personally
>>> > > > > > > > > > > > favour what is done currently.
>>> > > > > > > > > > > >
>>> > > > > > > > > > > > On Thu, Aug 17, 2023 at 9:34 AM Ismael Juma <
>>> > > > > me@ismaeljuma.com>
>>> > > > > > > > > wrote:
>>> > > > > > > > > > > >
>>> > > > > > > > > > > > > Hi,
>>> > > > > > > > > > > > >
>>> > > > > > > > > > > > > Thanks for the KIP. The problem outlined here is
>>> a
>>> > > great
>>> > > > > > > example
>>> > > > > > > > > why we
>>> > > > > > > > > > > > > should be using a record-like structure to pass
>>> the
>>> > > > > parameters
>>> > > > > > > to a
>>> > > > > > > > > > > > method
>>> > > > > > > > > > > > > like this. Then we can add more parameters
>>> without
>>> > > having
>>> > > > > to
>>> > > > > > > > > introduce
>>> > > > > > > > > > > > new
>>> > > > > > > > > > > > > methods. Have we considered this option?
>>> > > > > > > > > > > > >
>>> > > > > > > > > > > > > Ismael
>>> > > > > > > > > > > > >
>>> > > > > > > > > > > > > On Mon, Aug 7, 2023 at 5:26 AM Jack Tomy <
>>> > > > > > > jacktomy107@gmail.com>
>>> > > > > > > > > > > wrote:
>>> > > > > > > > > > > > >
>>> > > > > > > > > > > > > > Hey everyone.
>>> > > > > > > > > > > > > >
>>> > > > > > > > > > > > > > I would like to call for a vote on KIP-953:
>>> > partition
>>> > > > > method
>>> > > > > > > to
>>> > > > > > > > > be
>>> > > > > > > > > > > > > > overloaded to accept headers as well.
>>> > > > > > > > > > > > > >
>>> > > > > > > > > > > > > > KIP :
>>> > > > > > > > > > > > > >
>>> > > > > > > > > > > > >
>>> > > > > > > > > > > >
>>> > > > > > > > > > >
>>> > > > > > > > >
>>> > > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937
>>> > > > > > > > > > > > > > Discussion thread :
>>> > > > > > > > > > > > > >
>>> > > > > > >
>>> https://lists.apache.org/thread/0f20kvfqkmhdqrwcb8vqgqn80szcrcdd
>>> > > > > > > > > > > > > >
>>> > > > > > > > > > > > > > Thanks
>>> > > > > > > > > > > > > > --
>>> > > > > > > > > > > > > > Best Regards
>>> > > > > > > > > > > > > > *Jack*
>>> > > > > > > > > > > > > >
>>> > > > > > > > > > > > >
>>> > > > > > > > > > > >
>>> > > > > > > > > > > >
>>> > > > > > > > > > > > --
>>> > > > > > > > > > > > Best Regards
>>> > > > > > > > > > > > *Jack*
>>> > > > > > > > > > > >
>>> > > > > > > > > > >
>>> > > > > > > > > >
>>> > > > > > > > > >
>>> > > > > > > > > > --
>>> > > > > > > > > > Best Regards
>>> > > > > > > > > > *Jack*
>>> > > > > > > > >
>>> > > > > > >
>>> > > > >
>>> > > >
>>> > >
>>> >
>>> >
>>> > --
>>> > Best Regards
>>> > *Jack*
>>> >
>>>
>>
>>
>> --
>> Best Regards
>> *Jack*
>>
>
>
> --
> Best Regards
> *Jack*
>
--
Best Regards
*Jack*
Re: [VOTE] KIP-953: partition method to be overloaded to accept headers as well.
Posted by Jack Tomy <ja...@gmail.com>.
A last reminder :(
On Sun, Oct 15, 2023 at 1:25 PM Jack Tomy <ja...@gmail.com> wrote:
> Hey contributors,
>
> Please vote or veto the proposal.
>
> Please don't ghost. Humble request.
>
> Thanks
>
> On Thu, Sep 28, 2023 at 7:23 AM Jack Tomy <ja...@gmail.com> wrote:
>
>> Bumping up.....
>>
>> On Sun, Sep 17, 2023 at 9:34 AM Jack Tomy <ja...@gmail.com> wrote:
>>
>>> Hey Ismael, Sagar and everyone,
>>>
>>> Sorry I seem to have interpreted the thread wrong. Before we go ahead
>>> with the DTO based approach I have few reasons not to go with it.
>>> a. It is not following the pattern we are following today. But here I
>>> agree that patterns are to be changed for good.
>>> b. The client is not supposed to modify the record object (This is my
>>> understanding, If this is not a necessary requirement, please call it
>>> out.), passing the entire object lets the client do that. To avoid that,
>>> there has to be a way to deep copy the record object each time, this adds
>>> unnecessary requirements on the record object to support the deepcopy
>>> implementation. I see a lot of complexity and coupling coming in here due
>>> to this N I believe it's a strong reason not to go ahead with the DTO
>>> approach.
>>>
>>> Please let me know what you think.
>>>
>>> Thanks.
>>>
>>>
>>>
>>>
>>>
>>> On Wed, Sep 6, 2023 at 7:06 AM Sagar <sa...@gmail.com> wrote:
>>>
>>>> Hey Jack,
>>>>
>>>> The way I interpreted this thread, it seems like there's more alignment
>>>> on
>>>> the DTO based approach. I spent some time on the suggestion that Ismael
>>>> had
>>>> regarding the usage of ProducerRecord. Did you get a chance to look at
>>>> the
>>>> reply I had posted and whether that makes sense? Also, checking out the
>>>> AdminClient APIs examples provided by Ismael will give you more context.
>>>> Let me know what you think.
>>>>
>>>> Thanks!
>>>> Sagar.
>>>>
>>>> On Thu, Aug 31, 2023 at 12:49 PM Jack Tomy <ja...@gmail.com>
>>>> wrote:
>>>>
>>>> > Hey everyone,
>>>> >
>>>> > As I see devs favouring the current style of implementation, and that
>>>> is
>>>> > inline with existing code. I would like to go ahead with the same
>>>> approach
>>>> > as mentioned in the KIP.
>>>> > Can I get a few more votes so that I can take the KIP forward.
>>>> >
>>>> > Thanks
>>>> >
>>>> >
>>>> >
>>>> > On Sun, Aug 27, 2023 at 1:38 PM Sagar <sa...@gmail.com>
>>>> wrote:
>>>> >
>>>> > > Hi Ismael,
>>>> > >
>>>> > > Thanks for pointing us towards the direction of a DTO based
>>>> approach. The
>>>> > > AdminClient examples seem very neat and extensible in that sense.
>>>> > > Personally, I was trying to think only along the lines of how the
>>>> current
>>>> > > Partitioner interface has been designed, i.e having all requisite
>>>> > > parameters as separate arguments (Topic, Key, Value etc).
>>>> > >
>>>> > > Regarding this question of yours:
>>>> > >
>>>> > > A more concrete question: did we consider having the method
>>>> `partition`
>>>> > > > take `ProduceRecord` as one of its parameters and `Cluster` as the
>>>> > other?
>>>> > >
>>>> > >
>>>> > > No, I don't think in the discussion thread it was brought up and as
>>>> I
>>>> > said
>>>> > > it appears that could be due to an attempt to keep the new method's
>>>> > > signature similar to the existing one within Partitioner. If I
>>>> understood
>>>> > > the intent of the question correctly, are you trying to hint here
>>>> that
>>>> > > `ProducerRecord` already contains all the arguments that the
>>>> `partition`
>>>> > > method accepts and also has a `headers` field within it. So,
>>>> instead of
>>>> > > adding another method for the `headers` field, why not create a new
>>>> > method
>>>> > > taking ProducerRecord directly?
>>>> > >
>>>> > > If my understanding is correct, then it seems like a very clean way
>>>> of
>>>> > > adding support for `headers`. Anyways, the partition method within
>>>> > > KafkaProducer already takes a ProducerRecord as an argument so that
>>>> makes
>>>> > > it easier. Keeping that in mind, should this new method's (which
>>>> takes a
>>>> > > ProducerRecord as an input) default implementation invoke the
>>>> existing
>>>> > > method ? One challenge I see there is that the existing partition
>>>> method
>>>> > > expects serialized keys and values while ProducerRecord doesn't have
>>>> > access
>>>> > > to those (It directly operates on K, V).
>>>> > >
>>>> > > Thanks!
>>>> > > Sagar.
>>>> > >
>>>> > >
>>>> > > On Sun, Aug 27, 2023 at 8:51 AM Ismael Juma <me...@ismaeljuma.com>
>>>> wrote:
>>>> > >
>>>> > > > A more concrete question: did we consider having the method
>>>> `partition`
>>>> > > > take `ProduceRecord` as one of its parameters and `Cluster` as the
>>>> > other?
>>>> > > >
>>>> > > > Ismael
>>>> > > >
>>>> > > > On Sat, Aug 26, 2023 at 12:50 PM Greg Harris
>>>> > > <greg.harris@aiven.io.invalid
>>>> > > > >
>>>> > > > wrote:
>>>> > > >
>>>> > > > > Hey Ismael,
>>>> > > > >
>>>> > > > > > The mention of "runtime" is specific to Connect. When it
>>>> comes to
>>>> > > > > clients,
>>>> > > > > one typically compiles and runs with the same version or runs
>>>> with a
>>>> > > > newer
>>>> > > > > version than the one used for compilation. This is standard
>>>> practice
>>>> > in
>>>> > > > > Java and not something specific to Kafka.
>>>> > > > >
>>>> > > > > When I said "older runtimes" I was being lazy, and should have
>>>> said
>>>> > > > > "older versions of clients at runtime," thank you for figuring
>>>> out
>>>> > > > > what I meant.
>>>> > > > >
>>>> > > > > I don't know how common it is to compile a partitioner against
>>>> one
>>>> > > > > version of clients, and then distribute and run the partitioner
>>>> with
>>>> > > > > older versions of clients and expect graceful degradation of
>>>> > features.
>>>> > > > > If you say that it is very uncommon and not something that we
>>>> should
>>>> > > > > optimize for, then I won't suggest otherwise.
>>>> > > > >
>>>> > > > > > With regards to the Admin APIs, they have been extended
>>>> several
>>>> > times
>>>> > > > > since introduction (naturally). One of them is:
>>>> > > > > >
>>>> > > > >
>>>> > > >
>>>> > >
>>>> >
>>>> https://github.com/apache/kafka/commit/1d22b0d70686aef5689b775ea2ea7610a37f3e8c
>>>> > > > >
>>>> > > > > Thanks for the example. I also see that includes a migration
>>>> from
>>>> > > > > regular arguments to the DTO style, consistent with your
>>>> > > > > recommendation here.
>>>> > > > >
>>>> > > > > I think the DTO style and the proposed additional argument
>>>> style are
>>>> > > > > both reasonable.
>>>> > > > >
>>>> > > > > Thanks,
>>>> > > > > Greg
>>>> > > > >
>>>> > > > > On Sat, Aug 26, 2023 at 9:46 AM Ismael Juma <me...@ismaeljuma.com>
>>>> > wrote:
>>>> > > > > >
>>>> > > > > > Hi Greg,
>>>> > > > > >
>>>> > > > > > The mention of "runtime" is specific to Connect. When it
>>>> comes to
>>>> > > > > clients,
>>>> > > > > > one typically compiles and runs with the same version or runs
>>>> with
>>>> > a
>>>> > > > > newer
>>>> > > > > > version than the one used for compilation. This is standard
>>>> > practice
>>>> > > in
>>>> > > > > > Java and not something specific to Kafka.
>>>> > > > > >
>>>> > > > > > With regards to the Admin APIs, they have been extended
>>>> several
>>>> > times
>>>> > > > > since
>>>> > > > > > introduction (naturally). One of them is:
>>>> > > > > >
>>>> > > > > >
>>>> > > > >
>>>> > > >
>>>> > >
>>>> >
>>>> https://github.com/apache/kafka/commit/1d22b0d70686aef5689b775ea2ea7610a37f3e8c
>>>> > > > > >
>>>> > > > > > Ismael
>>>> > > > > >
>>>> > > > > > On Sat, Aug 26, 2023 at 8:29 AM Greg Harris
>>>> > > > <greg.harris@aiven.io.invalid
>>>> > > > > >
>>>> > > > > > wrote:
>>>> > > > > >
>>>> > > > > > > Hey Ismael,
>>>> > > > > > >
>>>> > > > > > > Thank you for clarifying where the DTO pattern is used
>>>> already, I
>>>> > > did
>>>> > > > > > > not have the admin methods in mind.
>>>> > > > > > >
>>>> > > > > > > > With the DTO approach, you don't create a new DTO, you
>>>> simply
>>>> > > add a
>>>> > > > > new
>>>> > > > > > > overloaded constructor and accessor to the DTO.
>>>> > > > > > >
>>>> > > > > > > With this variant, partitioner implementations would
>>>> receive a
>>>> > > > > > > `NoSuchMethodException` when trying to access newer methods
>>>> in
>>>> > > older
>>>> > > > > > > runtimes. Do we expect the interface implementers will
>>>> maintain
>>>> > the
>>>> > > > > > > try-catch to support backwards-compatibility?
>>>> > > > > > > Fortunately here the Headers type already exists, but in the
>>>> > future
>>>> > > > if
>>>> > > > > > > a new subtype is added at the same time as the change to
>>>> the DTO
>>>> > is
>>>> > > > > > > made, interface implementers will need to be careful to
>>>> avoid
>>>> > > > > > > NoClassDefFoundErrors.
>>>> > > > > > > We used this "add a new method" style extension in
>>>> > > > > > >
>>>> > > > > > >
>>>> > > > >
>>>> > > >
>>>> > >
>>>> >
>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-610%3A+Error+Reporting+in+Sink+Connectors
>>>> > > > > > > and had to be very specific in recommending how users
>>>> interact
>>>> > with
>>>> > > > > > > the new extension point, and there ended up being lots of
>>>> sharp
>>>> > > edges
>>>> > > > > > > in practice.
>>>> > > > > > >
>>>> > > > > > > Do you have any examples of a DTO-based API that has been
>>>> > extended
>>>> > > > > > > since it was initially implemented? I'm not familiar with
>>>> the
>>>> > > > > > > evolution of the Admin APIs.
>>>> > > > > > >
>>>> > > > > > > Thanks!
>>>> > > > > > > Greg
>>>> > > > > > >
>>>> > > > > > > On Sat, Aug 26, 2023 at 6:45 AM Ismael Juma <
>>>> me@ismaeljuma.com>
>>>> > > > wrote:
>>>> > > > > > > >
>>>> > > > > > > > Hi Greg,
>>>> > > > > > > >
>>>> > > > > > > > The point is that the approach proposed here introduces
>>>> > > complexity
>>>> > > > > > > forever.
>>>> > > > > > > > Each new user of this interface that needs access to the
>>>> > > parameters
>>>> > > > > not
>>>> > > > > > > > exposed originally needs to implement the abstract method
>>>> with
>>>> > an
>>>> > > > > empty
>>>> > > > > > > > implementation and it needs to override whichever
>>>> additional
>>>> > > > default
>>>> > > > > they
>>>> > > > > > > > care about (this KIP introduces a second method, but
>>>> future
>>>> > KIPs
>>>> > > > > would
>>>> > > > > > > > introduce additional methods for new parameters). One
>>>> would
>>>> > never
>>>> > > > > design
>>>> > > > > > > > the interface like this from the start.
>>>> > > > > > > >
>>>> > > > > > > > With the DTO approach, you don't create a new DTO, you
>>>> simply
>>>> > > add a
>>>> > > > > new
>>>> > > > > > > > overloaded constructor and accessor to the DTO. The
>>>> > implementers
>>>> > > of
>>>> > > > > the
>>>> > > > > > > > interface still have a single method (two here since we
>>>> made a
>>>> > > > > mistake
>>>> > > > > > > > originally) and they can decide which of the values from
>>>> the
>>>> > DTO
>>>> > > > they
>>>> > > > > > > would
>>>> > > > > > > > like to access. This approach has been the recommended
>>>> approach
>>>> > > for
>>>> > > > > years
>>>> > > > > > > > and it's how the Admin apis work (they're the most recent
>>>> > > client).
>>>> > > > An
>>>> > > > > > > > example:
>>>> > > > > > > >
>>>> > > > > > > > createTopics(Collection<NewTopic> newTopics,
>>>> > CreateTopicsOptions
>>>> > > > > > > options);
>>>> > > > > > > >
>>>> > > > > > > > This makes it easy to add new fields to `NewTopic` or
>>>> > > > > > > `CreateTopicsOptions`.
>>>> > > > > > > >
>>>> > > > > > > > Ismael
>>>> > > > > > > >
>>>> > > > > > > > On Wed, Aug 23, 2023 at 11:48 AM Greg Harris
>>>> > > > > > > <gr...@aiven.io.invalid>
>>>> > > > > > > > wrote:
>>>> > > > > > > >
>>>> > > > > > > > > Hey Jack,
>>>> > > > > > > > >
>>>> > > > > > > > > The design of this KIP is also consistent with the way
>>>> header
>>>> > > > > support
>>>> > > > > > > > > was added to Connect:
>>>> > > > > > > > >
>>>> > > > > > > > >
>>>> > > > > > >
>>>> > > > >
>>>> > > >
>>>> > >
>>>> >
>>>> https://cwiki.apache.org/confluence/display/KAFKA/KIP-440%3A+Extend+Connect+Converter+to+support+headers
>>>> > > > > > > > > I think making argument for precedent here is
>>>> reasonable.
>>>> > > > > > > > >
>>>> > > > > > > > > Hi Ismael,
>>>> > > > > > > > >
>>>> > > > > > > > > Can you expand what you mean by "without breaking
>>>> > > > compatibility"? I
>>>> > > > > > > > > think the approach proposed here (a default method)
>>>> would be
>>>> > > > > backwards
>>>> > > > > > > > > compatible. If an implementation wishes to make use of
>>>> the
>>>> > new
>>>> > > > > > > > > signature, they can override the new method and the
>>>> version
>>>> > of
>>>> > > > > Kafka
>>>> > > > > > > > > will determine which implementation is used without
>>>> instance
>>>> > > > > checking,
>>>> > > > > > > > > reflection, or exceptions.
>>>> > > > > > > > >
>>>> > > > > > > > > I believe that when you pass a DTO, that some sort of
>>>> > instance
>>>> > > > > > > > > checking, reflection, or exceptions would be required
>>>> for the
>>>> > > > > > > > > Partitioner to determine whether additional information
>>>> is
>>>> > > > present.
>>>> > > > > > > > > For example, if we wished to add some information X to
>>>> the
>>>> > > > > partitioner
>>>> > > > > > > > > in the future, the caller could pass either a
>>>> `PartitionInfo`
>>>> > > or
>>>> > > > > > > > > `PartitionInfoWithX` DTO instance, and the callee could
>>>> use
>>>> > an
>>>> > > > > > > > > `instanceof` check and a cast before accessing the X
>>>> > > information.
>>>> > > > > That
>>>> > > > > > > > > seems to be more machinery for the Partitioner
>>>> implementation
>>>> > > to
>>>> > > > > > > > > manage as compared to maintaining multiple methods,
>>>> which may
>>>> > > > just
>>>> > > > > be
>>>> > > > > > > > > one-line calls to other methods.
>>>> > > > > > > > >
>>>> > > > > > > > > Please let me know if I've misunderstood your DTO
>>>> design.
>>>> > > > > > > > >
>>>> > > > > > > > > Thanks!
>>>> > > > > > > > > Greg
>>>> > > > > > > > >
>>>> > > > > > > > > On Tue, Aug 22, 2023 at 9:33 PM Jack Tomy <
>>>> > > jacktomy107@gmail.com
>>>> > > > >
>>>> > > > > > > wrote:
>>>> > > > > > > > > >
>>>> > > > > > > > > > Hi Ismael,
>>>> > > > > > > > > >
>>>> > > > > > > > > > That would be totally different from the pattern
>>>> currently
>>>> > > > being
>>>> > > > > > > followed
>>>> > > > > > > > > > in all the interfaces, for example serializer.
>>>> > > > > > > > > > I personally don't favour that either. Let's see if
>>>> the
>>>> > > > community
>>>> > > > > > > has any
>>>> > > > > > > > > > opinions on the same.
>>>> > > > > > > > > >
>>>> > > > > > > > > > Hey everyone, please share your thoughts on using a
>>>> DTO
>>>> > > instead
>>>> > > > > of
>>>> > > > > > > > > separate
>>>> > > > > > > > > > params for the interface.
>>>> > > > > > > > > >
>>>> > > > > > > > > > Thanks.
>>>> > > > > > > > > >
>>>> > > > > > > > > > On Mon, Aug 21, 2023 at 8:06 PM Ismael Juma <
>>>> > > me@ismaeljuma.com
>>>> > > > >
>>>> > > > > > > wrote:
>>>> > > > > > > > > >
>>>> > > > > > > > > > > Hi Jack,
>>>> > > > > > > > > > >
>>>> > > > > > > > > > > I mean a DTO. That means you can add additional
>>>> > parameters
>>>> > > > > later
>>>> > > > > > > > > without
>>>> > > > > > > > > > > breaking compatibility. The current proposal would
>>>> result
>>>> > > in
>>>> > > > > yet
>>>> > > > > > > > > another
>>>> > > > > > > > > > > method each time we need to add parameters.
>>>> > > > > > > > > > >
>>>> > > > > > > > > > > Ismael
>>>> > > > > > > > > > >
>>>> > > > > > > > > > > On Sun, Aug 20, 2023 at 4:53 AM Jack Tomy <
>>>> > > > > jacktomy107@gmail.com>
>>>> > > > > > > > > wrote:
>>>> > > > > > > > > > >
>>>> > > > > > > > > > > > Hey Ismael,
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > > > Are you suggesting to pass a param like a DTO or
>>>> you
>>>> > are
>>>> > > > > > > suggesting
>>>> > > > > > > > > to
>>>> > > > > > > > > > > pass
>>>> > > > > > > > > > > > the record object?
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > > > I would also like to hear other devs' opinions on
>>>> this
>>>> > > as I
>>>> > > > > > > > > personally
>>>> > > > > > > > > > > > favour what is done currently.
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > > > On Thu, Aug 17, 2023 at 9:34 AM Ismael Juma <
>>>> > > > > me@ismaeljuma.com>
>>>> > > > > > > > > wrote:
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > > > > Hi,
>>>> > > > > > > > > > > > >
>>>> > > > > > > > > > > > > Thanks for the KIP. The problem outlined here
>>>> is a
>>>> > > great
>>>> > > > > > > example
>>>> > > > > > > > > why we
>>>> > > > > > > > > > > > > should be using a record-like structure to pass
>>>> the
>>>> > > > > parameters
>>>> > > > > > > to a
>>>> > > > > > > > > > > > method
>>>> > > > > > > > > > > > > like this. Then we can add more parameters
>>>> without
>>>> > > having
>>>> > > > > to
>>>> > > > > > > > > introduce
>>>> > > > > > > > > > > > new
>>>> > > > > > > > > > > > > methods. Have we considered this option?
>>>> > > > > > > > > > > > >
>>>> > > > > > > > > > > > > Ismael
>>>> > > > > > > > > > > > >
>>>> > > > > > > > > > > > > On Mon, Aug 7, 2023 at 5:26 AM Jack Tomy <
>>>> > > > > > > jacktomy107@gmail.com>
>>>> > > > > > > > > > > wrote:
>>>> > > > > > > > > > > > >
>>>> > > > > > > > > > > > > > Hey everyone.
>>>> > > > > > > > > > > > > >
>>>> > > > > > > > > > > > > > I would like to call for a vote on KIP-953:
>>>> > partition
>>>> > > > > method
>>>> > > > > > > to
>>>> > > > > > > > > be
>>>> > > > > > > > > > > > > > overloaded to accept headers as well.
>>>> > > > > > > > > > > > > >
>>>> > > > > > > > > > > > > > KIP :
>>>> > > > > > > > > > > > > >
>>>> > > > > > > > > > > > >
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > >
>>>> > > > > > > > >
>>>> > > > > > >
>>>> > > > >
>>>> > > >
>>>> > >
>>>> >
>>>> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=263424937
>>>> > > > > > > > > > > > > > Discussion thread :
>>>> > > > > > > > > > > > > >
>>>> > > > > > >
>>>> https://lists.apache.org/thread/0f20kvfqkmhdqrwcb8vqgqn80szcrcdd
>>>> > > > > > > > > > > > > >
>>>> > > > > > > > > > > > > > Thanks
>>>> > > > > > > > > > > > > > --
>>>> > > > > > > > > > > > > > Best Regards
>>>> > > > > > > > > > > > > > *Jack*
>>>> > > > > > > > > > > > > >
>>>> > > > > > > > > > > > >
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > > > --
>>>> > > > > > > > > > > > Best Regards
>>>> > > > > > > > > > > > *Jack*
>>>> > > > > > > > > > > >
>>>> > > > > > > > > > >
>>>> > > > > > > > > >
>>>> > > > > > > > > >
>>>> > > > > > > > > > --
>>>> > > > > > > > > > Best Regards
>>>> > > > > > > > > > *Jack*
>>>> > > > > > > > >
>>>> > > > > > >
>>>> > > > >
>>>> > > >
>>>> > >
>>>> >
>>>> >
>>>> > --
>>>> > Best Regards
>>>> > *Jack*
>>>> >
>>>>
>>>
>>>
>>> --
>>> Best Regards
>>> *Jack*
>>>
>>
>>
>> --
>> Best Regards
>> *Jack*
>>
>
>
> --
> Best Regards
> *Jack*
>
--
Best Regards
*Jack*