Hi, I've noticed that ElectLeadersResult is also the only result class to be declared final, which means that mocking frameworks like Mockito cannot mock it. So I'd like to also propose to remove that modifier at the same time as deprecating the public use of the constructors. I've updated the KIP to mention this.
Since there haven't been any other comments I'll start a vote soon. Kind regards, Tom On Thu, Sep 9, 2021 at 5:01 PM Josep Prat <josep.p...@aiven.io.invalid> wrote: > Hi Tom, > > Thanks for your elaborate explanation. I agree with you, it doesn't seem > like the best option right now. Thanks for updating the KIP with this > rejected alternative. > > Best, > ——— > Josep Prat > > Aiven Deutschland GmbH > > Immanuelkirchstraße 26, 10405 Berlin > > Amtsgericht Charlottenburg, HRB 209739 B > > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > > m: +491715557497 > > w: aiven.io > > e: josep.p...@aiven.io > > On Thu, Sep 9, 2021, 17:37 Tom Bentley <tbent...@redhat.com> wrote: > > > Hi Josep, > > > > Thanks for the question! I did consider it briefly: > > > > * As you mention, it would also be quite a lot more code (interfaces + > > impls) for relatively small protection. > > * It wouldn't simplify life for people wanting to mock the Admin client. > It > > would break code at runtime for people who (for whatever reason) are > > already using reflection to instantiate the package-private constructors. > > * I don't _think_ it would be binary compatible for clients that were > just > > invoking methods on Result instances (specifically, I _think_ the JVM > > rejects bytecode that uses invokevirtual on an interface, the byte code > > should be using invokeinterface instead). Maybe that's not a huge problem > > because people should not be substituting a Kafka 4.x jar for a 3.x one > at > > runtime. > > > > So on balance I'm inclined to the simpler approach in the KIP. > > > > If we _did_ decide to switch to interface in 4.0, deprecating the > > constructors would be a first step anyway, so it would still make sense > to > > deprecate now. The difference would be what happens for 4.0 (or 5.0...): > > Either changing the access modifier or introducing interfaces. Therefore > > it's something we could change our minds on before that major release, if > > there were other motivations for using interfaces. > > > > I've added this to the rejected alternatives. > > > > Kind regards, > > > > Tom > > > > On Thu, Sep 9, 2021 at 3:54 PM Josep Prat <josep.p...@aiven.io.invalid> > > wrote: > > > > > Hi Tom, > > > Thanks for the KIP, > > > I have one question. Have you considered converting those classes to > > > interfaces? This would also solve the problem. I must admit that the > > change > > > is way bigger this way, but I would argue it offers a cleaner > distinction > > > and it offers more safety against mistakenly making methods public that > > > shouldn't be. > > > > > > > > > Thanks! > > > ——— > > > Josep Prat > > > > > > Aiven Deutschland GmbH > > > > > > Immanuelkirchstraße 26, 10405 Berlin > > > > > > Amtsgericht Charlottenburg, HRB 209739 B > > > > > > Geschäftsführer: Oskari Saarenmaa & Hannu Valtonen > > > > > > m: +491715557497 > > > > > > w: aiven.io > > > > > > e: josep.p...@aiven.io > > > > > > On Thu, Sep 9, 2021, 16:25 Tom Bentley <tbent...@redhat.com> wrote: > > > > > > > Hi, > > > > > > > > I've written a small KIP-774 that proposes to deprecate public access > > to > > > > the Admin client's *Result constructors: > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-774%3A+Deprecate+public+access+to+Admin+client%27s+*Result+constructors > > > > > > > > I'd be grateful for any comments you may have. > > > > > > > > Kind regards, > > > > > > > > Tom > > > > > > > > > >
