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