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