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 <dong...@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 <cmcc...@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>*
>
