kafka-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Konstantine Karantasis <konstant...@confluent.io>
Subject Re: [DISCUSS] KIP-495: Dynamically Adjust Log Levels in Connect
Date Mon, 19 Aug 2019 22:20:09 GMT
Thanks Arjun, the example is useful!

My point when I mentioned the restrictions around log4j is that this is
information is significant and IMO needs to be included in the KIP.

Speaking of its relevance to KIP-412, I think a reference would be nice
too.

Konstantine



On Thu, Aug 15, 2019 at 4:00 PM Arjun Satish <arjun.satish@gmail.com> wrote:

> Hey Konstantine,
>
> Thanks for the feedback.
>
> re: the use of log4j, yes, the proposed changes will only work if log4j is
> available in runtime. We will not add the mBean if log4j is not available
> in classpath. If we change from log4j 1 to 2, that would involve another
> KIP, and it would need to update the changes proposed in this KIP and
> others (KIP-412, for instance).
>
> re: use of Object types, I've changed it from Boolean to the primitive type
> for setLogLevel. We are changing the signature of the old method this way,
> but since it never returned null, this should be fine.
>
> re: example usage, I've added some screenshot on how this feature would be
> used with jconsole.
>
> Hope this works!
>
> Thanks very much,
> Arjun
>
> On Wed, Aug 14, 2019 at 6:42 AM Konstantine Karantasis <
> konstantine@confluent.io> wrote:
>
> > And one thing I forgot is also related to Chris's comment above. I agree
> > that an example on how a user is expected to set the log level (for
> > instance to DEBUG) would be nice, even if it's showing only one out of
> the
> > many possible ways to achieve that.
> >
> > - Konstantine
> >
> > On Wed, Aug 14, 2019 at 4:38 PM Konstantine Karantasis <
> > konstantine@confluent.io> wrote:
> >
> > >
> > > Thanks Arjun for tackling the need to support this very useful feature.
> > >
> > > One thing I noticed while reading the KIP is that I would have loved to
> > > see more info regarding how this proposal depends on the underlying
> > logging
> > > APIs and implementations. For instance, my understanding is that slf4j
> > can
> > > not be leveraged and that the logging framework needs to be pegged to
> > log4j
> > > explicitly (or another logging implementation). Correct me if I'm
> wrong,
> > > but if such a dependency is introduced I believe it's worth mentioning.
> > >
> > > Additionally, if the above is correct, there are differences in log4j's
> > > APIs between version 1 and version 2. In version 2, Logger#setLevel
> > method
> > > has been removed from the Logger interface and in order to set the log
> > > level programmatically the Configurator class needs to used, which as
> > > stated in the FAQ (
> > > https://logging.apache.org/log4j/2.x/faq.html#reconfig_level_from_code
> )
> > > it's not part of log4j2's public API. Is this a concern? I believe that
> > > even if these are implementation specific details for the wrappers
> > > introduced by this KIP (which to a certain extent they are), a mention
> in
> > > the KIP text and a few references would be useful to understand the
> > changes
> > > and the dependencies introduced by this proposal.
> > >
> > > And a few minor comments:
> > > - Is there any specific reason that object types were preferred in the
> > > proposed interface compared to primitive types? My understanding is
> that
> > > `null` is not expected as a return value.
> > > - Related to the above, I think it'd be nice for the javadoc to mention
> > > when a parameter is not expected to be `null` with an appropriate
> comment
> > > (e.g. foo bar etc; may not be null)
> > >
> > > Cheers,
> > > Konstantine
> > >
> > > On Tue, Aug 6, 2019 at 9:34 AM Cyrus Vafadari <cyrus@confluent.io>
> > wrote:
> > >
> > >> This looks like a useful feature, the strategy makes sense, and the
> KIP
> > is
> > >> thorough and nicely written. Thanks!
> > >>
> > >> Cyrus
> > >>
> > >> On Thu, Aug 1, 2019, 12:40 PM Chris Egerton <chrise@confluent.io>
> > wrote:
> > >>
> > >> > Thanks Arjun! Looks good to me.
> > >> >
> > >> > On Thu, Aug 1, 2019 at 12:33 PM Arjun Satish <
> arjun.satish@gmail.com>
> > >> > wrote:
> > >> >
> > >> > > Thanks for the feedback, Chris!
> > >> > >
> > >> > > Yes, the example is pretty much how Connect will use the new
> > feature.
> > >> > > Tweaked the section to make this more clear.
> > >> > >
> > >> > > Best,
> > >> > >
> > >> > > On Fri, Jul 26, 2019 at 11:52 AM Chris Egerton <
> chrise@confluent.io
> > >
> > >> > > wrote:
> > >> > >
> > >> > > > Hi Arjun,
> > >> > > >
> > >> > > > This looks great. The changes to public interface are pretty
> small
> > >> and
> > >> > > > moving the Log4jController class into the clients package
seems
> > like
> > >> > the
> > >> > > > right way to go. One question I have--it looks like the
purpose
> of
> > >> this
> > >> > > KIP
> > >> > > > is to enable dynamic setting of log levels in the Connect
> > framework,
> > >> > but
> > >> > > > it's not clear how the Connect framework will use that new
> > utility.
> > >> Is
> > >> > > the
> > >> > > > "Example Usage" section (which involves invoking the utility
> with
> > a
> > >> > > > namespace of "kafka.connect") actually meant to be part
of the
> > >> proposed
> > >> > > > changes to public interface?
> > >> > > >
> > >> > > > Cheers,
> > >> > > >
> > >> > > > Chris
> > >> > > >
> > >> > > > On Mon, Jul 22, 2019 at 11:03 PM Arjun Satish <
> > >> arjun.satish@gmail.com>
> > >> > > > wrote:
> > >> > > >
> > >> > > > > Hi everyone.
> > >> > > > >
> > >> > > > > I'd like to propose the following KIP to implement
changing
> log
> > >> > levels
> > >> > > on
> > >> > > > > the fly in Connect workers:
> > >> > > > >
> > >> > > > >
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-495%3A+Dynamically+Adjust+Log+Levels+in+Connect
> > >> > > > >
> > >> > > > > Would like to hear your thoughts on this.
> > >> > > > >
> > >> > > > > Thanks very much,
> > >> > > > > Arjun
> > >> > > > >
> > >> > > >
> > >> > >
> > >> >
> > >>
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message