-1 If the goal is not to have so much config logging, let's add a configuration key that suppresses log4j in AbstractConfig. Then plugins can set that configuration if they want before invoking the superclass constructors.
(Or in the interest of allowing people to be verbose if they want, maybe let's say that we have a configuration key that sets the log4j configuration logging to TRACE rather than what it is now). This will avoid exposing our guts to the world (our configuration guts are very, very ugly in my opinion). It will be helpful to end-users as well as plugin authors -- maybe people want this. And it doesn't require the subclassing hack to get access to it. best, Colin On Fri, Nov 3, 2023, at 17:06, Matthias J. Sax wrote: > Sophie reads my mind well, but I also won't object if majority if people > thinks it's desirable to have it public (it's does not really hurt to > have them public). > > I just personally think, we should optimize for "end users" and they > should not need it -- and thus, keeping the API surface area as small as > possible seems desirable (and don't generate JavaDocs for protected > methods...). Maybe it's less of an issue for clients, but given my > experience with Kafka Streams, and it large API, I prefer to guide users > by avoiding "leaky" abstractions. > > -Matthias > > > > On 11/3/23 4:34 PM, Chris Egerton wrote: >> 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 >>>>>>> >>>>>> >>>>> >>>> >>> >>
