Hi Ismael,
These are all fair questions. I did a quick review of how these
constructors have changed. There are a few cases where extra constructor
parameters were added, so overloading the constructor would've been a
trivial way to be backwards compatible. The more interesting cases are
those which change generic type arguments:
CreateTopicsResult changed in 5d0052fe00c9071095b872b3b6ab6b1b455c9e20
- CreateTopicsResult(Map<String, KafkaFuture<Void>> futures) {
+ CreateTopicsResult(Map<String, KafkaFuture<TopicMetadataAndConfig>>
futures) {
We would have needed to have an overloaded constructor with an unused
parameter to disambiguate the signature, and then live with that in the API
even if the original construct were deprecated and removed. Probably the
implementation would have required two, rather than one field for two maps
in order that #values() still worked when the constructor was only given
the old-style futures.
DescribeLogDirsResult changed in 819cd454f9b3078515a86e43a4486794df0a7971
- DescribeLogDirsResult(Map<Integer, KafkaFuture<Map<String,
LogDirInfo>>> futures) {
+ DescribeLogDirsResult(Map<Integer, KafkaFuture<Map<String,
LogDirDescription>>> futures) {
this.futures = futures;
}
Not a source compatible change. We would have needed to have an overloaded
constructor with an unused parameter to disambiguate the signature, and
then live with that. This was part of work to fix the use of an internal
class leaking into the API, rather like the KafkaFutureImpl issue spotted
in time for Kafka 3.0. It had to deprecate methods that returned
LogDirInfo. I think the implementation could have used thenApply to convert
inner Map values from LogDirInfo to LogDirDescription before delegating to
the new constructor.
DescribeTopicsResult changed in 1d22b0d70686aef5689b775ea2ea7610a37f3e8c
+ @Deprecated
protected DescribeTopicsResult(Map<String,
KafkaFuture<TopicDescription>> futures) {
- this.futures = futures;
+ this(null, futures);
+ }
+
+ // VisibleForTesting
+ protected DescribeTopicsResult(Map<Uuid,
KafkaFuture<TopicDescription>> topicIdFutures, Map<String,
KafkaFuture<TopicDescription>> nameFutures) {
+ if (topicIdFutures != null && nameFutures != null)
+ throw new IllegalArgumentException("topicIdFutures and
nameFutures cannot both be specified.");
+ if (topicIdFutures == null && nameFutures == null)
+ throw new IllegalArgumentException("topicIdFutures and
nameFutures cannot both be null.");
+ this.topicIdFutures = topicIdFutures;
+ this.nameFutures = nameFutures;
+ }
Part of the topic ids work, clearly care has been taken to preserve
backwards compatibility and this was backwards compatible.
DeleteTopicsResult changed again in
cee2e975d124da0a2bdc0065a3172ce31e036fa0
- protected DeleteTopicsResult(Map<String, KafkaFuture<Void>> futures) {
+ protected DeleteTopicsResult(Map<Uuid, KafkaFuture<Void>>
topicIdFutures, Map<String, KafkaFuture<Void>> nameFutures) {
Again part of the topic ids work, but this would've been an incompatible
change, though I'm guessing overloading could have solved it.
In summary, I don't think any of these would've been a show-stopper, but
two of them would have necessitated unused constructor parameters to
disambiguate overloaded constructors.
I suppose that could be avoided by the use of private constructors and
public static factory methods (different method names means there's no
overloading, so no ambiguity in when signatures need to differ only by
generic type argument). But we'd have to choose names for those static
methods -- that's easy when you know that DeleteTopicsResult supports
by-name and by-id variants, but we didn't know that originally, so it's
likely those methods would end up with poor names. So it's not much of an
improvement over public constructors, in my opinion.
While we're thinking about the ease of testing our APIs, the absence of a
public API for creating a failed KafkaFuture is an annoyance. Currently to
test an exceptional path people have to delve into KafkaFutureImpl.
Thoughts?
Tom
On Tue, Sep 21, 2021 at 7:51 PM Ismael Juma <ism...@juma.me.uk> wrote:
> Hi Tom,
>
> I think these are all fair points. I was actually part of the group that
> decided that constructors should not be public. Since then I've noticed
> that many users find this limiting. I think we should make testability a
> key criteria for our APIs. Requiring a mocking library for data classes
> like this is suboptimal.
>
> To understand the value of the package private constructors, I have some
> questions:
>
> 1. For the cases where we evolved the constructors, how much did we gain?
> 2. Would a deprecation + removal be possible?
> 3. Did we have to deprecate some methods anyway?
>
> Ismael
>
> On Tue, Sep 21, 2021 at 10:07 AM Tom Bentley <tbent...@redhat.com> wrote:
>
> > Hi Ismael,
> >
> > I agree that that is a laudable aim, but I couldn't see a good way of
> > achieving that while simultaneously allowing us the ability to evolve the
> > constructor signatures without breaking (or at least having to reason
> about
> > the compatibility impact of) test code which instantiates them. Hence the
> > need to decide whether this is worth addressing, and if so, how.
> >
> > In the KIP I prioritised the ability to evolve the API because I think
> that
> > was the original intent behind package-private, and I think that's a
> valid
> > reason to hide them. If it weren't possible to mock them at all I would
> > weigh the trade-off differently.
> >
> > I wouldn't argue if there was a consensus for making the constructors
> > public. The overall point would be having consistency. Speaking
> personally,
> > I think the constructor issue in KAFKA-13276 snuck past me because I
> didn't
> > notice the public modifier having internalised "all those constructors
> are
> > package-private".
> >
> > Did you have any bright ideas for how to provide more ergonomic mocking
> > without exposing the constructors? Or do you just value the mocking above
> > the evolvability in this case?
> >
> > Kind regards,
> >
> > Tom
> >
> > On Tue, Sep 21, 2021 at 1:21 PM Ismael Juma <ism...@juma.me.uk> wrote:
> >
> > > Hi Tom,
> > >
> > > You say:
> > >
> > > "While the creation of Admin mocks with package constructors is not
> > > _ergonomic_, it is _possible_. The example code in KIP-692 requires two
> > > line of codes for each result instance."
> > >
> > > Should we not be aiming to make it ergonomic?
> > >
> > > Ismael
> > >
> > > On Thu, Sep 9, 2021 at 7:25 AM 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
> > > >
> > >
> >
>
