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 >> > > > > >> > > > >> > > >> > >> >
