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 < konstant...@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 < > konstant...@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 <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 > >> > > > > > >> > > > > >> > > > >> > > >> > > >
