No objections, I'm +1 ether way. On Fri, Nov 3, 2023, 18:50 Sophie Blee-Goldman <sop...@responsive.dev> wrote:
> I am fine with making them public. Of course in that case we should also > change the corresponding constructors in ConsumerConfig, AdminConfig, and > StreamsConfig from protected to public as well, to be consistent. But > Matthias seems to feel that these should never be instantiated by a user > and that the correct course of action would be to move in the opposite > direction. > > I don't personally feel strongly either way -- honestly I had thought it > was an abuse of internal APIs to extend the other Config classes in order > to access the protected constructor and disable logging. So I would be > happy to officially pull it into the public API with all-public > constructors, because I do feel it is valid/useful to be able to > instantiate these objects. We do so in order to access config values in a > way that accounts for any overrides on top of the default, for example when > multiple overrides are in play (eg user overrides on top of framework > overrides on top of Kafka Streams overrides on top of > Consumer/Consumer/Admin client defaults). Using them is also (slightly) > more type-safe than going through a Properties or config Map<> > > Any objections to expanding the KIP to the ConsumerConfig, AdminConfig, and > StreamsConfig constructors and making them public as well? From Matthias or > otherwise? > > On Fri, Nov 3, 2023 at 11:09 AM Ismael Juma <m...@ismaeljuma.com> wrote: > > > It seems wrong to require inheritance for this and we already have a > public > > constructor. I would make both of them public. > > > > Ismael > > > > On Fri, Nov 3, 2023 at 10:47 AM Matthias J. Sax <mj...@apache.org> > wrote: > > > > > +1 (binding) > > > > > > > > > About "why not public" question: > > > > > > I think we need to distinguish between "end users" who create a > producer > > > instance, and "external parties" that might implement their own > > > `Producer` (or wrap/extend `KafkaProducer`). > > > > > > In the end, I would not expect an "end user" to actually call `new > > > ProducerConfig` to begin with. If one creates a `KafkaProducer` they > > > pass the config via a `Map` or `Properties`, and the producer creates > > > `ProducerConfig` internally only. -- Thus, there is no need to make it > > > `public`. (To this end, I don't actually understand why there is public > > > `ProducerConfig` constructors to begin with -- sounds like a leaky > > > abstraction to me.) > > > > > > On the other hand, if a "third party" implements `Producer` interface > to > > > ship their own producer implementation, they might want to create > > > `ProducerConfig` internally, so for them it's different, but still, > they > > > don't need public access because they can extend `ProducerConfig`, too > > > for this case). -- To me, this falls into the category "simple thing > > > should be easy, and hard things should be possible). > > > > > > > > > -Matthias > > > > > > > > > On 11/3/23 6:06 AM, Ismael Juma wrote: > > > > Hi Sophie, > > > > > > > > I was trying to understand the goal of the change and it's not > totally > > > > clear to me. If the goal is to allow third party applications to > > > customize > > > > the logging behavior, why is the method protected instead of public? > > > > > > > > Ismael > > > > > > > > On Thu, Nov 2, 2023 at 9:55 PM Sophie Blee-Goldman < > > > sop...@responsive.dev> > > > > wrote: > > > > > > > >> Hey all, > > > >> > > > >> This is a trivial one-liner change that it was determined should go > > > through > > > >> a KIP during the PR review process (see this thread > > > >> <https://github.com/apache/kafka/pull/14681#discussion_r1378591228> > > for > > > >> context). Since the change itself was already reviewed and approved > > I'm > > > >> skipping the discussion thread and bringing it to a vote right away, > > > but of > > > >> course I'm open to feedback and can create a discussion thread if > > there > > > is > > > >> need for it. > > > >> > > > >> The change itself is simply adding the `protected` modifier to the > > > >> ProducerConfig constructor that allows for silencing the config > > logging. > > > >> This just brings the ProducerConfig in alignment with the other > client > > > >> configs, all of which already had this constructor as protected. > > > >> > > > >> KIP: > > > >> > > > >> > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-998%3A+Give+ProducerConfig%28props%2C+doLog%29+constructor+protected+access > > > >> PR: https://github.com/apache/kafka/pull/14681 > > > >> > > > >> Thanks! > > > >> Sophie > > > >> > > > > > > > > > >
