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