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 <cy...@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 <chr...@confluent.io> wrote: > > > Thanks Arjun! Looks good to me. > > > > On Thu, Aug 1, 2019 at 12:33 PM Arjun Satish <arjun.sat...@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 <chr...@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.sat...@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 > > > > > > > > > > > > > > >
