You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Dongjin Lee <do...@apache.org> on 2020/07/08 14:41:37 UTC
Re: [DISCUSS] KIP-621: Deprecate and replace DescribeLogDirsResult.all()
and .values()
Hi Tom,
Thanks for taking this issue. I opened a PR for this issue earlier, but
your KIP was submitted first. So I closed my one
<https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866169>
.
I have a question: for consistency with other methods, how about
maintaining the signature of DescribeLogDirsResult#[all, values]? There is
an alternative approach to deprecate DescribeLogDirsResponse.[LogDirInfo,
ReplicaInfo]. (Please have a look at my proposal.)
Best,
Dongjin
+1. I updated the link to the discussion thread on your KIP document.
On 2020/06/29 09:31:53, Tom Bentley <t....@redhat.com> wrote:
> Hi,>
>
> Does anyone have any comments about this? If not I'll likely start a
vote>
> in a couple of days.>
>
> Kind regards,>
>
> Tom>
>
> On Mon, Jun 8, 2020 at 4:56 PM Tom Bentley <tb...@redhat.com> wrote:>
>
> > Hi all,>
> >>
> > I've opened a small KIP seeking to deprecate and replace a couple of>
> > methods of DescribeLogDirsResult which refer to internal classes in
their>
> > return type.>
> >>
> >
https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862109>
> >>
> > Please take a look if you have the time.>
> >>
> > Kind regards,>
> >>
> > Tom>
> >>
>
Re: [DISCUSS] KIP-621: Deprecate and replace DescribeLogDirsResult.all()
and .values()
Posted by Dongjin Lee <do...@apache.org>.
Hi Tom and Colin,
I see. Thanks for the comprehensive explanation. I learned a lot.
Although my approach includes a new member field
`DescribeLogDirsResult.LogDirInfo.tpToReplicaInfos` to keep binary
compatibility, but anyway, you are right - it does not worth except small
consistency gain.
Best,
Dongjin
On Fri, Jul 10, 2020 at 3:07 PM Colin McCabe <cm...@apache.org> wrote:
> Incidentally, whenever we do a hard compatibility break with this API, we
> should probably get rid of the function that iterates over every replica on
> the broker. That's not a scalable API for the future. We also probably
> should have an API to list just the directories that are moving, which is
> what most tools care about anyway.
>
> best,
> Colin
>
>
> On Thu, Jul 9, 2020, at 23:04, Colin McCabe wrote:
> > Yeah. The issue with subclassing is that it's a source compatibility
> > break, although not (I think) a binary compatibility break. I don't
> > think it's really worth it given that it leaves us with a weird class
> > hierarchy, and we still need to do a hard compatibility break later to
> > fix the real problem with either solution.
> >
> > best,
> > Colin
> >
> >
> > On Thu, Jul 9, 2020, at 08:39, Tom Bentley wrote:
> > > Hi Dongjin,
> > >
> > > Thanks for updating the link and sorry that you put effort into writing
> > > your own KIP for this. I was aware of KAFKA-8794, but since the PR was
> > > about javadocing the internal classes I felt it wasn't quite the same
> > > thing. All the same, I should have pinged you on KAFKA-8794, sorry.
> > >
> > > I did consider subclassing too. Initially I thought it would be binary
> > > incompatible, but thinking about it more I realise that, perhaps, it's
> > > binary compatible due to erasure, (I'd have to test it to be sure, or
> > > perhaps you've already done so, and can confirm).
> > >
> > > But I think there are a few other reasons to be considered:
> > >
> > > * The change would not be source compatible (people would have to
> change
> > > their source code) because the signature of all() and values() use type
> > > arguments invariantly, i.e. KafkaFuture<Map<Integer, Map<String,
> > > DescribeLogDirsResult .LogDirInfo>>> is not a subtype of
> > > KafkaFuture<Map<Integer, Map<String,
> DescribeLogDirsResponse.LogDirInfo>>>
> > > so you'd get a compile time error as use sites where you were trying to
> > > assign the new result to the old type (and this applies to all the
> > > intermediate generic types too).
> > > * There's also the fact that `replicaInfos` and `error` is accessed
> via a
> > > field, rather than a method. To fix that you would need to:
> > > * Add methods to the new subclasses and deprecate the old fields
> (so
> > > that a user gets a warning if accessing the field, even though they've
> > > eliminated the old class name from the code).
> > > * As Colin has mentioned, you'd have to deprecate those classes and
> fields,
> > > even though they're not really deprecated from the internal PoV,
> they're
> > > only deprecated from the external PoV.
> > > * Using subclassing in this way would introduce a different kind of
> > > inconsistency: other APIs don't use internal classes at all, even via
> > > inheritance.
> > > * Using static member classes is inconsistent with other Admin APIs
> such as
> > > TopicDescription (though I think this is trivially fixed in your
> proposal).
> > >
> > > The loss of consistency wrt to the method names in the approach
> advocated
> > > by KIP-621 could eventually be removed by adding "new" values() and
> all()
> > > methods back with those names but the new types. Indeed, I _think_ this
> > > could be done in a major release without breaking SemVer.
> > >
> > > All considered, I think it's cleaner to deprecate the methods and
> create
> > > new classes completely independent of the internal ones.
> > >
> > > Kind regards,
> > >
> > > Tom
> > >
> > > On Thu, Jul 9, 2020 at 4:26 PM Dongjin Lee <do...@apache.org> wrote:
> > >
> > > > Hi Colin,
> > > >
> > > >
> > > > Thanks for the quick response. Sure, the problem is that the internal
> > > > classes (DescribeLogDirsResponse.[LogDirInfo, ReplicaInfo], or 'the
> old
> > > > ones') are exposed to the public API, not the class itself. You are
> right.
> > > >
> > > >
> > > > However, deprecating DescribeLogDirsResult#[all, values] and
> defining the
> > > > new methods causes another problem: consistency. As of 2.5, most of
> the
> > > > admin result classes (listed below) have 'all' and 'values' methods.
> Since
> > > > these methods are public APIs, I think keeping consistency would be
> better.
> > > >
> > > >
> > > >
> > > > -
> > > >
> > > >
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateTopicsResult.html
> > > > -
> > > >
> > > >
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterPartitionReassignmentsResult.html
> > > > -
> > > >
> > > >
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateAclsResult.html
> > > > -
> > > >
> > > >
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterConfigsResult.html
> > > >
> > > >
> > > > In contrast, my approach - deprecating the old ones and defining the
> new
> > > > ones (DescribeLogDirsResult.[LogDirInfo, ReplicaInfo]) that extends
> the old
> > > > counterpart - not only not breaking the consistency but also gives
> the
> > > > users the message that they have to use the new ones.
> > > >
> > > >
> > > > So, the overall process would be:
> > > >
> > > >
> > > >
> > > > 1. Deprecate the old ones and make DescribeLogDirsResult#[all,
> values]
> > > > to return the new ones, without changing the return type. Since
> the new
> > > > ones extend the old ones, it does not cause any problems.
> > > > 2. Since deprecation message guides to move to the new ones, the
> uses
> > > > will migrate to use the new classes.
> > > > 3. After a time, do the following:
> > > > 1. Change the return type of DescribeLogDirsResult#[all,
> values] from
> > > > old ones to the new ones, with a notice. Since we already
> deprecated
> > > > the
> > > > old ones, most users would already be moved into the new ones.
> So it
> > > > does
> > > > not occur any problems.
> > > > 2. *Hide the old ones from the public, with removing the
> deprecated
> > > > annotation.* Since it is not exposed to the public anymore, it
> also
> > > > does not cause any problems - now we can use it freely in the
> > > > internals.
> > > >
> > > >
> > > > Is my intention clear now? If not, please have a brief look at my
> draft
> > > > implementation here <https://github.com/apache/kafka/pull/7204/files
> >.
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Dongjin
> > > >
> > > > On Thu, Jul 9, 2020 at 4:34 AM Colin McCabe <cm...@apache.org>
> wrote:
> > > >
> > > > > Hi Dongjin,
> > > > >
> > > > > Hmm. I'm not sure I follow. How does deprecating
> > > > > DescribeLogDirsResponse.LogDirInfo help here? The issue is not so
> much
> > > > the
> > > > > class, but the fact that it's exposed as a public API. So it seems
> > > > > appropriate to deprecate the methods that return it, but not the
> class
> > > > > itself, since we'll continue to use it internally.
> > > > >
> > > > > best,
> > > > > Colin
> > > > >
> > > > >
> > > > > On Wed, Jul 8, 2020, at 07:41, Dongjin Lee wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > Thanks for taking this issue. I opened a PR for this issue
> earlier, but
> > > > > > your KIP was submitted first. So I closed my one
> > > > > > <
> > > > >
> > > >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866169
> > > > > >
> > > > > > .
> > > > > >
> > > > > > I have a question: for consistency with other methods, how about
> > > > > > maintaining the signature of DescribeLogDirsResult#[all,
> values]? There
> > > > > is
> > > > > > an alternative approach to deprecate
> > > > DescribeLogDirsResponse.[LogDirInfo,
> > > > > > ReplicaInfo]. (Please have a look at my proposal.)
> > > > > >
> > > > > > Best,
> > > > > > Dongjin
> > > > > >
> > > > > > +1. I updated the link to the discussion thread on your KIP
> document.
> > > > > >
> > > > > > On 2020/06/29 09:31:53, Tom Bentley <t....@redhat.com> wrote:
> > > > > > > Hi,>
> > > > > > >
> > > > > > > Does anyone have any comments about this? If not I'll likely
> start a
> > > > > > vote>
> > > > > > > in a couple of days.>
> > > > > > >
> > > > > > > Kind regards,>
> > > > > > >
> > > > > > > Tom>
> > > > > > >
> > > > > > > On Mon, Jun 8, 2020 at 4:56 PM Tom Bentley <tb...@redhat.com>
> > > > wrote:>
> > > > > > >
> > > > > > > > Hi all,>
> > > > > > > >>
> > > > > > > > I've opened a small KIP seeking to deprecate and replace a
> couple
> > > > of>
> > > > > > > > methods of DescribeLogDirsResult which refer to internal
> classes in
> > > > > > their>
> > > > > > > > return type.>
> > > > > > > >>
> > > > > > > >
> > > > > >
> > > > >
> > > >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862109
> > > > > >
> > > > > > > >>
> > > > > > > > Please take a look if you have the time.>
> > > > > > > >>
> > > > > > > > Kind regards,>
> > > > > > > >>
> > > > > > > > Tom>
> > > > > > > >>
> > > > > > >
> > > > > >
> > > > >
> > > >
> > > >
> > > > --
> > > > *Dongjin Lee*
> > > >
> > > > *A hitchhiker in the mathematical world.*
> > > >
> > > >
> > > >
> > > >
> > > > *github: <http://goog_969573159/>github.com/dongjinleekr
> > > > <https://github.com/dongjinleekr>keybase:
> https://keybase.io/dongjinleekr
> > > > <https://keybase.io/dongjinleekr>linkedin:
> kr.linkedin.com/in/dongjinleekr
> > > > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > > > speakerdeck.com/dongjin
> > > > <https://speakerdeck.com/dongjin>*
> > > >
> > >
> >
>
--
*Dongjin Lee*
*A hitchhiker in the mathematical world.*
*github: <http://goog_969573159/>github.com/dongjinleekr
<https://github.com/dongjinleekr>keybase: https://keybase.io/dongjinleekr
<https://keybase.io/dongjinleekr>linkedin: kr.linkedin.com/in/dongjinleekr
<https://kr.linkedin.com/in/dongjinleekr>speakerdeck: speakerdeck.com/dongjin
<https://speakerdeck.com/dongjin>*
Re: [DISCUSS] KIP-621: Deprecate and replace DescribeLogDirsResult.all() and .values()
Posted by Colin McCabe <cm...@apache.org>.
Incidentally, whenever we do a hard compatibility break with this API, we should probably get rid of the function that iterates over every replica on the broker. That's not a scalable API for the future. We also probably should have an API to list just the directories that are moving, which is what most tools care about anyway.
best,
Colin
On Thu, Jul 9, 2020, at 23:04, Colin McCabe wrote:
> Yeah. The issue with subclassing is that it's a source compatibility
> break, although not (I think) a binary compatibility break. I don't
> think it's really worth it given that it leaves us with a weird class
> hierarchy, and we still need to do a hard compatibility break later to
> fix the real problem with either solution.
>
> best,
> Colin
>
>
> On Thu, Jul 9, 2020, at 08:39, Tom Bentley wrote:
> > Hi Dongjin,
> >
> > Thanks for updating the link and sorry that you put effort into writing
> > your own KIP for this. I was aware of KAFKA-8794, but since the PR was
> > about javadocing the internal classes I felt it wasn't quite the same
> > thing. All the same, I should have pinged you on KAFKA-8794, sorry.
> >
> > I did consider subclassing too. Initially I thought it would be binary
> > incompatible, but thinking about it more I realise that, perhaps, it's
> > binary compatible due to erasure, (I'd have to test it to be sure, or
> > perhaps you've already done so, and can confirm).
> >
> > But I think there are a few other reasons to be considered:
> >
> > * The change would not be source compatible (people would have to change
> > their source code) because the signature of all() and values() use type
> > arguments invariantly, i.e. KafkaFuture<Map<Integer, Map<String,
> > DescribeLogDirsResult .LogDirInfo>>> is not a subtype of
> > KafkaFuture<Map<Integer, Map<String, DescribeLogDirsResponse.LogDirInfo>>>
> > so you'd get a compile time error as use sites where you were trying to
> > assign the new result to the old type (and this applies to all the
> > intermediate generic types too).
> > * There's also the fact that `replicaInfos` and `error` is accessed via a
> > field, rather than a method. To fix that you would need to:
> > * Add methods to the new subclasses and deprecate the old fields (so
> > that a user gets a warning if accessing the field, even though they've
> > eliminated the old class name from the code).
> > * As Colin has mentioned, you'd have to deprecate those classes and fields,
> > even though they're not really deprecated from the internal PoV, they're
> > only deprecated from the external PoV.
> > * Using subclassing in this way would introduce a different kind of
> > inconsistency: other APIs don't use internal classes at all, even via
> > inheritance.
> > * Using static member classes is inconsistent with other Admin APIs such as
> > TopicDescription (though I think this is trivially fixed in your proposal).
> >
> > The loss of consistency wrt to the method names in the approach advocated
> > by KIP-621 could eventually be removed by adding "new" values() and all()
> > methods back with those names but the new types. Indeed, I _think_ this
> > could be done in a major release without breaking SemVer.
> >
> > All considered, I think it's cleaner to deprecate the methods and create
> > new classes completely independent of the internal ones.
> >
> > Kind regards,
> >
> > Tom
> >
> > On Thu, Jul 9, 2020 at 4:26 PM Dongjin Lee <do...@apache.org> wrote:
> >
> > > Hi Colin,
> > >
> > >
> > > Thanks for the quick response. Sure, the problem is that the internal
> > > classes (DescribeLogDirsResponse.[LogDirInfo, ReplicaInfo], or 'the old
> > > ones') are exposed to the public API, not the class itself. You are right.
> > >
> > >
> > > However, deprecating DescribeLogDirsResult#[all, values] and defining the
> > > new methods causes another problem: consistency. As of 2.5, most of the
> > > admin result classes (listed below) have 'all' and 'values' methods. Since
> > > these methods are public APIs, I think keeping consistency would be better.
> > >
> > >
> > >
> > > -
> > >
> > > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateTopicsResult.html
> > > -
> > >
> > > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterPartitionReassignmentsResult.html
> > > -
> > >
> > > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateAclsResult.html
> > > -
> > >
> > > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterConfigsResult.html
> > >
> > >
> > > In contrast, my approach - deprecating the old ones and defining the new
> > > ones (DescribeLogDirsResult.[LogDirInfo, ReplicaInfo]) that extends the old
> > > counterpart - not only not breaking the consistency but also gives the
> > > users the message that they have to use the new ones.
> > >
> > >
> > > So, the overall process would be:
> > >
> > >
> > >
> > > 1. Deprecate the old ones and make DescribeLogDirsResult#[all, values]
> > > to return the new ones, without changing the return type. Since the new
> > > ones extend the old ones, it does not cause any problems.
> > > 2. Since deprecation message guides to move to the new ones, the uses
> > > will migrate to use the new classes.
> > > 3. After a time, do the following:
> > > 1. Change the return type of DescribeLogDirsResult#[all, values] from
> > > old ones to the new ones, with a notice. Since we already deprecated
> > > the
> > > old ones, most users would already be moved into the new ones. So it
> > > does
> > > not occur any problems.
> > > 2. *Hide the old ones from the public, with removing the deprecated
> > > annotation.* Since it is not exposed to the public anymore, it also
> > > does not cause any problems - now we can use it freely in the
> > > internals.
> > >
> > >
> > > Is my intention clear now? If not, please have a brief look at my draft
> > > implementation here <https://github.com/apache/kafka/pull/7204/files>.
> > >
> > >
> > > Thanks,
> > >
> > > Dongjin
> > >
> > > On Thu, Jul 9, 2020 at 4:34 AM Colin McCabe <cm...@apache.org> wrote:
> > >
> > > > Hi Dongjin,
> > > >
> > > > Hmm. I'm not sure I follow. How does deprecating
> > > > DescribeLogDirsResponse.LogDirInfo help here? The issue is not so much
> > > the
> > > > class, but the fact that it's exposed as a public API. So it seems
> > > > appropriate to deprecate the methods that return it, but not the class
> > > > itself, since we'll continue to use it internally.
> > > >
> > > > best,
> > > > Colin
> > > >
> > > >
> > > > On Wed, Jul 8, 2020, at 07:41, Dongjin Lee wrote:
> > > > > Hi Tom,
> > > > >
> > > > > Thanks for taking this issue. I opened a PR for this issue earlier, but
> > > > > your KIP was submitted first. So I closed my one
> > > > > <
> > > >
> > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866169
> > > > >
> > > > > .
> > > > >
> > > > > I have a question: for consistency with other methods, how about
> > > > > maintaining the signature of DescribeLogDirsResult#[all, values]? There
> > > > is
> > > > > an alternative approach to deprecate
> > > DescribeLogDirsResponse.[LogDirInfo,
> > > > > ReplicaInfo]. (Please have a look at my proposal.)
> > > > >
> > > > > Best,
> > > > > Dongjin
> > > > >
> > > > > +1. I updated the link to the discussion thread on your KIP document.
> > > > >
> > > > > On 2020/06/29 09:31:53, Tom Bentley <t....@redhat.com> wrote:
> > > > > > Hi,>
> > > > > >
> > > > > > Does anyone have any comments about this? If not I'll likely start a
> > > > > vote>
> > > > > > in a couple of days.>
> > > > > >
> > > > > > Kind regards,>
> > > > > >
> > > > > > Tom>
> > > > > >
> > > > > > On Mon, Jun 8, 2020 at 4:56 PM Tom Bentley <tb...@redhat.com>
> > > wrote:>
> > > > > >
> > > > > > > Hi all,>
> > > > > > >>
> > > > > > > I've opened a small KIP seeking to deprecate and replace a couple
> > > of>
> > > > > > > methods of DescribeLogDirsResult which refer to internal classes in
> > > > > their>
> > > > > > > return type.>
> > > > > > >>
> > > > > > >
> > > > >
> > > >
> > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862109
> > > > >
> > > > > > >>
> > > > > > > Please take a look if you have the time.>
> > > > > > >>
> > > > > > > Kind regards,>
> > > > > > >>
> > > > > > > Tom>
> > > > > > >>
> > > > > >
> > > > >
> > > >
> > >
> > >
> > > --
> > > *Dongjin Lee*
> > >
> > > *A hitchhiker in the mathematical world.*
> > >
> > >
> > >
> > >
> > > *github: <http://goog_969573159/>github.com/dongjinleekr
> > > <https://github.com/dongjinleekr>keybase: https://keybase.io/dongjinleekr
> > > <https://keybase.io/dongjinleekr>linkedin: kr.linkedin.com/in/dongjinleekr
> > > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > > speakerdeck.com/dongjin
> > > <https://speakerdeck.com/dongjin>*
> > >
> >
>
Re: [DISCUSS] KIP-621: Deprecate and replace DescribeLogDirsResult.all() and .values()
Posted by Colin McCabe <cm...@apache.org>.
Yeah. The issue with subclassing is that it's a source compatibility break, although not (I think) a binary compatibility break. I don't think it's really worth it given that it leaves us with a weird class hierarchy, and we still need to do a hard compatibility break later to fix the real problem with either solution.
best,
Colin
On Thu, Jul 9, 2020, at 08:39, Tom Bentley wrote:
> Hi Dongjin,
>
> Thanks for updating the link and sorry that you put effort into writing
> your own KIP for this. I was aware of KAFKA-8794, but since the PR was
> about javadocing the internal classes I felt it wasn't quite the same
> thing. All the same, I should have pinged you on KAFKA-8794, sorry.
>
> I did consider subclassing too. Initially I thought it would be binary
> incompatible, but thinking about it more I realise that, perhaps, it's
> binary compatible due to erasure, (I'd have to test it to be sure, or
> perhaps you've already done so, and can confirm).
>
> But I think there are a few other reasons to be considered:
>
> * The change would not be source compatible (people would have to change
> their source code) because the signature of all() and values() use type
> arguments invariantly, i.e. KafkaFuture<Map<Integer, Map<String,
> DescribeLogDirsResult .LogDirInfo>>> is not a subtype of
> KafkaFuture<Map<Integer, Map<String, DescribeLogDirsResponse.LogDirInfo>>>
> so you'd get a compile time error as use sites where you were trying to
> assign the new result to the old type (and this applies to all the
> intermediate generic types too).
> * There's also the fact that `replicaInfos` and `error` is accessed via a
> field, rather than a method. To fix that you would need to:
> * Add methods to the new subclasses and deprecate the old fields (so
> that a user gets a warning if accessing the field, even though they've
> eliminated the old class name from the code).
> * As Colin has mentioned, you'd have to deprecate those classes and fields,
> even though they're not really deprecated from the internal PoV, they're
> only deprecated from the external PoV.
> * Using subclassing in this way would introduce a different kind of
> inconsistency: other APIs don't use internal classes at all, even via
> inheritance.
> * Using static member classes is inconsistent with other Admin APIs such as
> TopicDescription (though I think this is trivially fixed in your proposal).
>
> The loss of consistency wrt to the method names in the approach advocated
> by KIP-621 could eventually be removed by adding "new" values() and all()
> methods back with those names but the new types. Indeed, I _think_ this
> could be done in a major release without breaking SemVer.
>
> All considered, I think it's cleaner to deprecate the methods and create
> new classes completely independent of the internal ones.
>
> Kind regards,
>
> Tom
>
> On Thu, Jul 9, 2020 at 4:26 PM Dongjin Lee <do...@apache.org> wrote:
>
> > Hi Colin,
> >
> >
> > Thanks for the quick response. Sure, the problem is that the internal
> > classes (DescribeLogDirsResponse.[LogDirInfo, ReplicaInfo], or 'the old
> > ones') are exposed to the public API, not the class itself. You are right.
> >
> >
> > However, deprecating DescribeLogDirsResult#[all, values] and defining the
> > new methods causes another problem: consistency. As of 2.5, most of the
> > admin result classes (listed below) have 'all' and 'values' methods. Since
> > these methods are public APIs, I think keeping consistency would be better.
> >
> >
> >
> > -
> >
> > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateTopicsResult.html
> > -
> >
> > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterPartitionReassignmentsResult.html
> > -
> >
> > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateAclsResult.html
> > -
> >
> > https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterConfigsResult.html
> >
> >
> > In contrast, my approach - deprecating the old ones and defining the new
> > ones (DescribeLogDirsResult.[LogDirInfo, ReplicaInfo]) that extends the old
> > counterpart - not only not breaking the consistency but also gives the
> > users the message that they have to use the new ones.
> >
> >
> > So, the overall process would be:
> >
> >
> >
> > 1. Deprecate the old ones and make DescribeLogDirsResult#[all, values]
> > to return the new ones, without changing the return type. Since the new
> > ones extend the old ones, it does not cause any problems.
> > 2. Since deprecation message guides to move to the new ones, the uses
> > will migrate to use the new classes.
> > 3. After a time, do the following:
> > 1. Change the return type of DescribeLogDirsResult#[all, values] from
> > old ones to the new ones, with a notice. Since we already deprecated
> > the
> > old ones, most users would already be moved into the new ones. So it
> > does
> > not occur any problems.
> > 2. *Hide the old ones from the public, with removing the deprecated
> > annotation.* Since it is not exposed to the public anymore, it also
> > does not cause any problems - now we can use it freely in the
> > internals.
> >
> >
> > Is my intention clear now? If not, please have a brief look at my draft
> > implementation here <https://github.com/apache/kafka/pull/7204/files>.
> >
> >
> > Thanks,
> >
> > Dongjin
> >
> > On Thu, Jul 9, 2020 at 4:34 AM Colin McCabe <cm...@apache.org> wrote:
> >
> > > Hi Dongjin,
> > >
> > > Hmm. I'm not sure I follow. How does deprecating
> > > DescribeLogDirsResponse.LogDirInfo help here? The issue is not so much
> > the
> > > class, but the fact that it's exposed as a public API. So it seems
> > > appropriate to deprecate the methods that return it, but not the class
> > > itself, since we'll continue to use it internally.
> > >
> > > best,
> > > Colin
> > >
> > >
> > > On Wed, Jul 8, 2020, at 07:41, Dongjin Lee wrote:
> > > > Hi Tom,
> > > >
> > > > Thanks for taking this issue. I opened a PR for this issue earlier, but
> > > > your KIP was submitted first. So I closed my one
> > > > <
> > >
> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866169
> > > >
> > > > .
> > > >
> > > > I have a question: for consistency with other methods, how about
> > > > maintaining the signature of DescribeLogDirsResult#[all, values]? There
> > > is
> > > > an alternative approach to deprecate
> > DescribeLogDirsResponse.[LogDirInfo,
> > > > ReplicaInfo]. (Please have a look at my proposal.)
> > > >
> > > > Best,
> > > > Dongjin
> > > >
> > > > +1. I updated the link to the discussion thread on your KIP document.
> > > >
> > > > On 2020/06/29 09:31:53, Tom Bentley <t....@redhat.com> wrote:
> > > > > Hi,>
> > > > >
> > > > > Does anyone have any comments about this? If not I'll likely start a
> > > > vote>
> > > > > in a couple of days.>
> > > > >
> > > > > Kind regards,>
> > > > >
> > > > > Tom>
> > > > >
> > > > > On Mon, Jun 8, 2020 at 4:56 PM Tom Bentley <tb...@redhat.com>
> > wrote:>
> > > > >
> > > > > > Hi all,>
> > > > > >>
> > > > > > I've opened a small KIP seeking to deprecate and replace a couple
> > of>
> > > > > > methods of DescribeLogDirsResult which refer to internal classes in
> > > > their>
> > > > > > return type.>
> > > > > >>
> > > > > >
> > > >
> > >
> > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862109
> > > >
> > > > > >>
> > > > > > Please take a look if you have the time.>
> > > > > >>
> > > > > > Kind regards,>
> > > > > >>
> > > > > > Tom>
> > > > > >>
> > > > >
> > > >
> > >
> >
> >
> > --
> > *Dongjin Lee*
> >
> > *A hitchhiker in the mathematical world.*
> >
> >
> >
> >
> > *github: <http://goog_969573159/>github.com/dongjinleekr
> > <https://github.com/dongjinleekr>keybase: https://keybase.io/dongjinleekr
> > <https://keybase.io/dongjinleekr>linkedin: kr.linkedin.com/in/dongjinleekr
> > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > speakerdeck.com/dongjin
> > <https://speakerdeck.com/dongjin>*
> >
>
Re: [DISCUSS] KIP-621: Deprecate and replace DescribeLogDirsResult.all()
and .values()
Posted by Tom Bentley <tb...@redhat.com>.
Hi Dongjin,
Thanks for updating the link and sorry that you put effort into writing
your own KIP for this. I was aware of KAFKA-8794, but since the PR was
about javadocing the internal classes I felt it wasn't quite the same
thing. All the same, I should have pinged you on KAFKA-8794, sorry.
I did consider subclassing too. Initially I thought it would be binary
incompatible, but thinking about it more I realise that, perhaps, it's
binary compatible due to erasure, (I'd have to test it to be sure, or
perhaps you've already done so, and can confirm).
But I think there are a few other reasons to be considered:
* The change would not be source compatible (people would have to change
their source code) because the signature of all() and values() use type
arguments invariantly, i.e. KafkaFuture<Map<Integer, Map<String,
DescribeLogDirsResult .LogDirInfo>>> is not a subtype of
KafkaFuture<Map<Integer, Map<String, DescribeLogDirsResponse.LogDirInfo>>>
so you'd get a compile time error as use sites where you were trying to
assign the new result to the old type (and this applies to all the
intermediate generic types too).
* There's also the fact that `replicaInfos` and `error` is accessed via a
field, rather than a method. To fix that you would need to:
* Add methods to the new subclasses and deprecate the old fields (so
that a user gets a warning if accessing the field, even though they've
eliminated the old class name from the code).
* As Colin has mentioned, you'd have to deprecate those classes and fields,
even though they're not really deprecated from the internal PoV, they're
only deprecated from the external PoV.
* Using subclassing in this way would introduce a different kind of
inconsistency: other APIs don't use internal classes at all, even via
inheritance.
* Using static member classes is inconsistent with other Admin APIs such as
TopicDescription (though I think this is trivially fixed in your proposal).
The loss of consistency wrt to the method names in the approach advocated
by KIP-621 could eventually be removed by adding "new" values() and all()
methods back with those names but the new types. Indeed, I _think_ this
could be done in a major release without breaking SemVer.
All considered, I think it's cleaner to deprecate the methods and create
new classes completely independent of the internal ones.
Kind regards,
Tom
On Thu, Jul 9, 2020 at 4:26 PM Dongjin Lee <do...@apache.org> wrote:
> Hi Colin,
>
>
> Thanks for the quick response. Sure, the problem is that the internal
> classes (DescribeLogDirsResponse.[LogDirInfo, ReplicaInfo], or 'the old
> ones') are exposed to the public API, not the class itself. You are right.
>
>
> However, deprecating DescribeLogDirsResult#[all, values] and defining the
> new methods causes another problem: consistency. As of 2.5, most of the
> admin result classes (listed below) have 'all' and 'values' methods. Since
> these methods are public APIs, I think keeping consistency would be better.
>
>
>
> -
>
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateTopicsResult.html
> -
>
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterPartitionReassignmentsResult.html
> -
>
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateAclsResult.html
> -
>
> https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterConfigsResult.html
>
>
> In contrast, my approach - deprecating the old ones and defining the new
> ones (DescribeLogDirsResult.[LogDirInfo, ReplicaInfo]) that extends the old
> counterpart - not only not breaking the consistency but also gives the
> users the message that they have to use the new ones.
>
>
> So, the overall process would be:
>
>
>
> 1. Deprecate the old ones and make DescribeLogDirsResult#[all, values]
> to return the new ones, without changing the return type. Since the new
> ones extend the old ones, it does not cause any problems.
> 2. Since deprecation message guides to move to the new ones, the uses
> will migrate to use the new classes.
> 3. After a time, do the following:
> 1. Change the return type of DescribeLogDirsResult#[all, values] from
> old ones to the new ones, with a notice. Since we already deprecated
> the
> old ones, most users would already be moved into the new ones. So it
> does
> not occur any problems.
> 2. *Hide the old ones from the public, with removing the deprecated
> annotation.* Since it is not exposed to the public anymore, it also
> does not cause any problems - now we can use it freely in the
> internals.
>
>
> Is my intention clear now? If not, please have a brief look at my draft
> implementation here <https://github.com/apache/kafka/pull/7204/files>.
>
>
> Thanks,
>
> Dongjin
>
> On Thu, Jul 9, 2020 at 4:34 AM Colin McCabe <cm...@apache.org> wrote:
>
> > Hi Dongjin,
> >
> > Hmm. I'm not sure I follow. How does deprecating
> > DescribeLogDirsResponse.LogDirInfo help here? The issue is not so much
> the
> > class, but the fact that it's exposed as a public API. So it seems
> > appropriate to deprecate the methods that return it, but not the class
> > itself, since we'll continue to use it internally.
> >
> > best,
> > Colin
> >
> >
> > On Wed, Jul 8, 2020, at 07:41, Dongjin Lee wrote:
> > > Hi Tom,
> > >
> > > Thanks for taking this issue. I opened a PR for this issue earlier, but
> > > your KIP was submitted first. So I closed my one
> > > <
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866169
> > >
> > > .
> > >
> > > I have a question: for consistency with other methods, how about
> > > maintaining the signature of DescribeLogDirsResult#[all, values]? There
> > is
> > > an alternative approach to deprecate
> DescribeLogDirsResponse.[LogDirInfo,
> > > ReplicaInfo]. (Please have a look at my proposal.)
> > >
> > > Best,
> > > Dongjin
> > >
> > > +1. I updated the link to the discussion thread on your KIP document.
> > >
> > > On 2020/06/29 09:31:53, Tom Bentley <t....@redhat.com> wrote:
> > > > Hi,>
> > > >
> > > > Does anyone have any comments about this? If not I'll likely start a
> > > vote>
> > > > in a couple of days.>
> > > >
> > > > Kind regards,>
> > > >
> > > > Tom>
> > > >
> > > > On Mon, Jun 8, 2020 at 4:56 PM Tom Bentley <tb...@redhat.com>
> wrote:>
> > > >
> > > > > Hi all,>
> > > > >>
> > > > > I've opened a small KIP seeking to deprecate and replace a couple
> of>
> > > > > methods of DescribeLogDirsResult which refer to internal classes in
> > > their>
> > > > > return type.>
> > > > >>
> > > > >
> > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862109
> > >
> > > > >>
> > > > > Please take a look if you have the time.>
> > > > >>
> > > > > Kind regards,>
> > > > >>
> > > > > Tom>
> > > > >>
> > > >
> > >
> >
>
>
> --
> *Dongjin Lee*
>
> *A hitchhiker in the mathematical world.*
>
>
>
>
> *github: <http://goog_969573159/>github.com/dongjinleekr
> <https://github.com/dongjinleekr>keybase: https://keybase.io/dongjinleekr
> <https://keybase.io/dongjinleekr>linkedin: kr.linkedin.com/in/dongjinleekr
> <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> speakerdeck.com/dongjin
> <https://speakerdeck.com/dongjin>*
>
Re: [DISCUSS] KIP-621: Deprecate and replace DescribeLogDirsResult.all()
and .values()
Posted by Dongjin Lee <do...@apache.org>.
Hi Colin,
Thanks for the quick response. Sure, the problem is that the internal
classes (DescribeLogDirsResponse.[LogDirInfo, ReplicaInfo], or 'the old
ones') are exposed to the public API, not the class itself. You are right.
However, deprecating DescribeLogDirsResult#[all, values] and defining the
new methods causes another problem: consistency. As of 2.5, most of the
admin result classes (listed below) have 'all' and 'values' methods. Since
these methods are public APIs, I think keeping consistency would be better.
-
https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateTopicsResult.html
-
https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterPartitionReassignmentsResult.html
-
https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/CreateAclsResult.html
-
https://kafka.apache.org/25/javadoc/org/apache/kafka/clients/admin/AlterConfigsResult.html
In contrast, my approach - deprecating the old ones and defining the new
ones (DescribeLogDirsResult.[LogDirInfo, ReplicaInfo]) that extends the old
counterpart - not only not breaking the consistency but also gives the
users the message that they have to use the new ones.
So, the overall process would be:
1. Deprecate the old ones and make DescribeLogDirsResult#[all, values]
to return the new ones, without changing the return type. Since the new
ones extend the old ones, it does not cause any problems.
2. Since deprecation message guides to move to the new ones, the uses
will migrate to use the new classes.
3. After a time, do the following:
1. Change the return type of DescribeLogDirsResult#[all, values] from
old ones to the new ones, with a notice. Since we already deprecated the
old ones, most users would already be moved into the new ones. So it does
not occur any problems.
2. *Hide the old ones from the public, with removing the deprecated
annotation.* Since it is not exposed to the public anymore, it also
does not cause any problems - now we can use it freely in the internals.
Is my intention clear now? If not, please have a brief look at my draft
implementation here <https://github.com/apache/kafka/pull/7204/files>.
Thanks,
Dongjin
On Thu, Jul 9, 2020 at 4:34 AM Colin McCabe <cm...@apache.org> wrote:
> Hi Dongjin,
>
> Hmm. I'm not sure I follow. How does deprecating
> DescribeLogDirsResponse.LogDirInfo help here? The issue is not so much the
> class, but the fact that it's exposed as a public API. So it seems
> appropriate to deprecate the methods that return it, but not the class
> itself, since we'll continue to use it internally.
>
> best,
> Colin
>
>
> On Wed, Jul 8, 2020, at 07:41, Dongjin Lee wrote:
> > Hi Tom,
> >
> > Thanks for taking this issue. I opened a PR for this issue earlier, but
> > your KIP was submitted first. So I closed my one
> > <
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866169
> >
> > .
> >
> > I have a question: for consistency with other methods, how about
> > maintaining the signature of DescribeLogDirsResult#[all, values]? There
> is
> > an alternative approach to deprecate DescribeLogDirsResponse.[LogDirInfo,
> > ReplicaInfo]. (Please have a look at my proposal.)
> >
> > Best,
> > Dongjin
> >
> > +1. I updated the link to the discussion thread on your KIP document.
> >
> > On 2020/06/29 09:31:53, Tom Bentley <t....@redhat.com> wrote:
> > > Hi,>
> > >
> > > Does anyone have any comments about this? If not I'll likely start a
> > vote>
> > > in a couple of days.>
> > >
> > > Kind regards,>
> > >
> > > Tom>
> > >
> > > On Mon, Jun 8, 2020 at 4:56 PM Tom Bentley <tb...@redhat.com> wrote:>
> > >
> > > > Hi all,>
> > > >>
> > > > I've opened a small KIP seeking to deprecate and replace a couple of>
> > > > methods of DescribeLogDirsResult which refer to internal classes in
> > their>
> > > > return type.>
> > > >>
> > > >
> >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862109
> >
> > > >>
> > > > Please take a look if you have the time.>
> > > >>
> > > > Kind regards,>
> > > >>
> > > > Tom>
> > > >>
> > >
> >
>
--
*Dongjin Lee*
*A hitchhiker in the mathematical world.*
*github: <http://goog_969573159/>github.com/dongjinleekr
<https://github.com/dongjinleekr>keybase: https://keybase.io/dongjinleekr
<https://keybase.io/dongjinleekr>linkedin: kr.linkedin.com/in/dongjinleekr
<https://kr.linkedin.com/in/dongjinleekr>speakerdeck: speakerdeck.com/dongjin
<https://speakerdeck.com/dongjin>*
Re: [DISCUSS] KIP-621: Deprecate and replace DescribeLogDirsResult.all() and .values()
Posted by Colin McCabe <cm...@apache.org>.
Hi Dongjin,
Hmm. I'm not sure I follow. How does deprecating DescribeLogDirsResponse.LogDirInfo help here? The issue is not so much the class, but the fact that it's exposed as a public API. So it seems appropriate to deprecate the methods that return it, but not the class itself, since we'll continue to use it internally.
best,
Colin
On Wed, Jul 8, 2020, at 07:41, Dongjin Lee wrote:
> Hi Tom,
>
> Thanks for taking this issue. I opened a PR for this issue earlier, but
> your KIP was submitted first. So I closed my one
> <https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158866169>
> .
>
> I have a question: for consistency with other methods, how about
> maintaining the signature of DescribeLogDirsResult#[all, values]? There is
> an alternative approach to deprecate DescribeLogDirsResponse.[LogDirInfo,
> ReplicaInfo]. (Please have a look at my proposal.)
>
> Best,
> Dongjin
>
> +1. I updated the link to the discussion thread on your KIP document.
>
> On 2020/06/29 09:31:53, Tom Bentley <t....@redhat.com> wrote:
> > Hi,>
> >
> > Does anyone have any comments about this? If not I'll likely start a
> vote>
> > in a couple of days.>
> >
> > Kind regards,>
> >
> > Tom>
> >
> > On Mon, Jun 8, 2020 at 4:56 PM Tom Bentley <tb...@redhat.com> wrote:>
> >
> > > Hi all,>
> > >>
> > > I've opened a small KIP seeking to deprecate and replace a couple of>
> > > methods of DescribeLogDirsResult which refer to internal classes in
> their>
> > > return type.>
> > >>
> > >
> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=158862109>
> > >>
> > > Please take a look if you have the time.>
> > >>
> > > Kind regards,>
> > >>
> > > Tom>
> > >>
> >
>