Hi Arjun, Thanks for the KIP. Do we really need a JMX-based API? Is there literally anyone in the world that wants to use JMX if they don't have to? I thought one of the major motivations of KIP-412 was how much of a pain JMX is.
Thanks, Jason On Mon, Aug 19, 2019 at 5:28 PM Arjun Satish <arjun.sat...@gmail.com> wrote: > Thanks, Konstantine. > > Updated the KIP with the restrictions around log4j and added references to > similar KIPs. > > Best, > > On Mon, Aug 19, 2019 at 3:20 PM Konstantine Karantasis < > konstant...@confluent.io> wrote: > > > 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 > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > >> > > > > > > > > > > > > > > >
