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 <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>* > > >
