Thanks for the updates, Arjun. If possible, it'd be great to have the KIP clarify a few things:
1) IIUC, the loggers returned by the GET methods are only those loggers that have been activated/used/set in the JVM. If this is the case, this should be specified. 2) It's possible to set a log level on an ancestor of other loggers, so we should also specify whether or not ancestors are included in the GET responses. Doing so would be helpful, but might not be feasible since two different descendants might have different log levels. Otherwise this looks good! Best regards, Randall On Mon, Sep 16, 2019 at 4:15 AM Arjun Satish <arjun.sat...@gmail.com> wrote: > Good catch, Randall. Yes, we should be able to set the level of any logger > given its name. If this is an ancestor, then the levels of all child > classes are updated. I updated the KIP to be more explicit about what > loggers we can set, and how they affect child classes, if any. > > Let me know what you think. > > Best, > > On Thu, Sep 12, 2019 at 5:02 PM Randall Hauch <rha...@gmail.com> wrote: > > > Thanks for the KIP, Arjun. It's going to be really nice to be able to set > > the log levels dynamically, especially through the REST API. > > > > However, I think it's not clear what behavior the KIP is actually > proposing > > with respect to which loggers are exposed (just those that are used, or > > common ancestors) and the behavior when I change the log level on a > > specific logger (is just that logger affected, or are descendants > affected, > > too). > > > > For example, in a Log4J configuration file we can set the logger for > > packages (e.g., `org.apache.kafka`, `org.apache.kafka.connect`, etc.) or > > classes (e.g., `org.apache.kafka.connect.runtime.WorkerSinkTask`). > Really, > > those are just conventions, because if the code created a logger for the > > "kafka.connect.worker" context then we could set that, too. So by > > convention, the loggers map to Kafka classes and packages. > > > > But it's unclear what behavior the KIP is proposing. Are the intermediate > > loggers such as all packages exposed as loggers? If so, if I set the > logger > > on `org.apache.kafka.connect.runtime`, will this set the log level for > all > > loggers below this? > > > > My concern is that if the behavior is (a) only concrete classes, and/or > (b) > > setting a log level for a specific logger sets only that logger, then > this > > deviates from what our users are familiar with when setting the log > levels > > in the Log4J configuration files, and would be a difficult user > experience > > if I have to set 30+ loggers rather than 1 or 2. > > > > On Thu, Sep 12, 2019 at 1:04 PM Jason Gustafson <ja...@confluent.io> > > wrote: > > > > > Thanks Arjun. +1 > > > > > > On Thu, Sep 12, 2019 at 9:58 AM Gwen Shapira <g...@confluent.io> > wrote: > > > > > > > The new REST API for logger management looks great to me. > > > > > > > > > > > > On Thu, Sep 12, 2019 at 8:36 AM Arjun Satish <arjun.sat...@gmail.com > > > > > > wrote: > > > > > > > > > > Bumping this thread. > > > > > > > > > > If there are no further comments, please add your votes here: > > > > > https://www.mail-archive.com/dev@kafka.apache.org/msg100313.html > > > > > > > > > > Thanks in advance, > > > > > Arjun > > > > > > > > > > On Fri, Sep 6, 2019 at 4:22 PM Arjun Satish < > arjun.sat...@gmail.com> > > > > wrote: > > > > > > > > > > > Thanks a lot, Jason! Answers inline. I'll also modify the kip to > > make > > > > > > these clear. > > > > > > > > > > > > On Fri, Sep 6, 2019 at 4:01 PM Jason Gustafson < > ja...@confluent.io > > > > > > > wrote: > > > > > > > > > > > >> Hi Arjun, > > > > > >> > > > > > >> The updated KIP looks good. Just a couple questions: > > > > > >> > > > > > >> 1. Is the /admin endpoint on the normal listener by default? If > > not, > > > > is > > > > > >> there a way to have it use the same listener? > > > > > >> > > > > > > > > > > > > Uses the normal listener by default. > > > > > > > > > > > > > > > > > >> 2. Changes to logging configuration are not intended to be > > > > persistent, is > > > > > >> that right? Also, I assume changes only apply to the worker that > > > > received > > > > > >> the request? > > > > > >> > > > > > > > > > > > > Changes will not be persistent and only apply to the worker that > > > > received > > > > > > the request. > > > > > > > > > > > > > > > > > >> Thanks, > > > > > >> Jason > > > > > >> > > > > > >> On Fri, Aug 30, 2019 at 1:25 AM Arjun Satish < > > > arjun.sat...@gmail.com> > > > > > >> wrote: > > > > > >> > > > > > >> > OK. I didn't realize the plan was to deprecate and remove the > > JMX > > > > > >> endpoint. > > > > > >> > KIP-412 says brokers will continue to expose the JMX API. JMX > > was > > > > > >> selected > > > > > >> > so all components could follow the brokers. In light of this, > I > > > > think we > > > > > >> > should simply aim for semantic equivalency across the > different > > > API > > > > for > > > > > >> > this functionality. > > > > > >> > > > > > > >> > REST is convenient for Connect. We can modify the KIP to have > a > > > > /admin > > > > > >> > endpoint, and /admin/loggers specifically for getting/setting > > the > > > > log > > > > > >> > levels of different loggers. The /admin/loggers will only > impact > > > > loggers > > > > > >> > running in the specific worker we target requests to, and upon > > > > > >> restarting > > > > > >> > the worker, these loggers will reset back to their original > > level. > > > > > >> > > > > > > >> > Since configuring the rest server already has multiple config > > > keys, > > > > I am > > > > > >> > inclined to bundle this /admin endpoint on to the same > listener, > > > and > > > > > >> > provide a single new config key that enables or disables the > > > entire > > > > > >> /admin > > > > > >> > endpoint. This keeps the initial approach simple and doesn't > > > require > > > > > >> users > > > > > >> > to configure/discover a new endpoint. > > > > > >> > > > > > > >> > If this works with you all, I can update the KIP. Please let > me > > > know > > > > > >> what > > > > > >> > you think. > > > > > >> > > > > > > >> > Thanks everyone. > > > > > >> > > > > > > >> > Best, > > > > > >> > > > > > > >> > On Thu, Aug 29, 2019 at 10:14 AM Colin McCabe < > > cmcc...@apache.org > > > > > > > > > >> wrote: > > > > > >> > > > > > > >> > > On Mon, Aug 26, 2019, at 14:03, Jason Gustafson wrote: > > > > > >> > > > Hi Arjun, > > > > > >> > > > > > > > > >> > > > From a high level, I feel like we are making light of the > > JMX > > > > api > > > > > >> > because > > > > > >> > > > it's convenient and the broker already has it. Personally > I > > > > would > > > > > >> take > > > > > >> > > the > > > > > >> > > > broker out of the picture. The JMX endpoint is not > something > > > we > > > > were > > > > > >> > > happy > > > > > >> > > > with, hence KIP-412. Ultimately I think we will deprecate > > and > > > > > >> remove it > > > > > >> > > and > > > > > >> > > > there's no point trying to standardize on a deprecated > > > > mechanism. > > > > > >> > > Thinking > > > > > >> > > > just about connect, we already have an HTTP endpoint. The > > > > default > > > > > >> > > position > > > > > >> > > > should be to add new APIs to it rather than introducing > > other > > > > > >> > mechanisms. > > > > > >> > > > The fewer ways you have to interact with a system, the > > better, > > > > > >> right? > > > > > >> > > > > > > > > >> > > > I think the main argument against a REST endpoint is > > basically > > > > that > > > > > >> > > > adjusting log levels is an administrative operation and > > > connect > > > > is > > > > > >> > > lacking > > > > > >> > > > an authorization framework to enforce administrative > access. > > > The > > > > > >> same > > > > > >> > > > argument applies to JMX, but it has the benefit that you > can > > > > specify > > > > > >> > > > different credentials and it is easier to isolate since it > > is > > > > > >> running > > > > > >> > on > > > > > >> > > a > > > > > >> > > > separate port. As you suggested, I think the same benefits > > > > could be > > > > > >> > > > achieved by having a separate /admin endpoint which is > > exposed > > > > > >> (perhaps > > > > > >> > > > optionally) on another listener. This is a pretty standard > > > > pattern. > > > > > >> If > > > > > >> > > > memory serves, dropwizard has something like this out of > the > > > > box. We > > > > > >> > > should > > > > > >> > > > think hard whether there are additional administrative > > > > capabilities > > > > > >> > that > > > > > >> > > we > > > > > >> > > > would ultimately need. The answer is probably yes, so > unless > > > we > > > > > >> want to > > > > > >> > > > double down on JMX, it might be worth thinking through the > > > > > >> implications > > > > > >> > > of > > > > > >> > > > an admin endpoint now so that we're not left with odd > > > > compatibility > > > > > >> > > baggage > > > > > >> > > > in the future. > > > > > >> > > > > > > > >> > > Hi Jason, > > > > > >> > > > > > > > >> > > I agree... I think Connect needs a REST admin API. There > will > > > > > >> probably > > > > > >> > be > > > > > >> > > a lot of other stuff that we'll want to add to it. > > > > > >> > > > > > > > >> > > best, > > > > > >> > > Colin > > > > > >> > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > Thanks, > > > > > >> > > > Jason > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > > > > > > >> > > > On Fri, Aug 23, 2019 at 5:38 PM Arjun Satish < > > > > > >> arjun.sat...@gmail.com> > > > > > >> > > wrote: > > > > > >> > > > > > > > > >> > > > > Jason, > > > > > >> > > > > > > > > > >> > > > > Thanks for your comments! > > > > > >> > > > > > > > > > >> > > > > I understand the usability issues with JMX that you > > mention. > > > > But > > > > > >> it > > > > > >> > was > > > > > >> > > > > chosen for the following reasons: > > > > > >> > > > > > > > > > >> > > > > 1. Cross-cutting functionality across different > components > > > > (Kafka > > > > > >> > > brokers, > > > > > >> > > > > Connect workers and even with Streams jobs). If we go > down > > > the > > > > > >> REST > > > > > >> > > route, > > > > > >> > > > > then brokers don't get this feature. > > > > > >> > > > > 2. Adding this to existing REST servers adds the > > > > whole-or-nothing > > > > > >> > > problem. > > > > > >> > > > > It's hard to disable an endpoint if the functionality is > > not > > > > > >> desired > > > > > >> > or > > > > > >> > > > > needs to be protected from users (Connect doesn't have > > ACLs > > > > which > > > > > >> > makes > > > > > >> > > > > this even harder to manage). Adding endpoints to > different > > > > > >> listeners > > > > > >> > > makes > > > > > >> > > > > configuring Connect harder (and it's already a hard > > problem > > > > as it > > > > > >> > is). > > > > > >> > > A > > > > > >> > > > > lot of the existing functionality there is driven around > > the > > > > > >> > connector > > > > > >> > > data > > > > > >> > > > > model (connectors, plugins, their statuses and so on). > > > Adding > > > > an > > > > > >> > > '/admin' > > > > > >> > > > > endpoint may be a way to go, but that has tremendous > > > > implications > > > > > >> (we > > > > > >> > > are > > > > > >> > > > > effectively adding an administration endpoint similar to > > the > > > > admin > > > > > >> > one > > > > > >> > > in > > > > > >> > > > > brokers), and probably requires a KIP of its own with > > > > discussions > > > > > >> > > catered > > > > > >> > > > > around just that. > > > > > >> > > > > 3. JMX is currently AK's default way to report metrics > and > > > > perform > > > > > >> > > other > > > > > >> > > > > operations. Changing log levels is typically a system > > > > level/admin > > > > > >> > > > > operation, and fits better there, instead of REST APIs > > > (which > > > > is > > > > > >> more > > > > > >> > > user > > > > > >> > > > > facing). > > > > > >> > > > > > > > > > >> > > > > Having said that, I'm happy to consider alternatives. > JMX > > > > seemed > > > > > >> to > > > > > >> > be > > > > > >> > > the > > > > > >> > > > > lowest hanging fruit. But if there are better ideas, we > > can > > > > > >> consider > > > > > >> > > them. > > > > > >> > > > > At the end of the day, when we download and run Kafka, > > there > > > > > >> should > > > > > >> > be > > > > > >> > > one > > > > > >> > > > > way to achieve the same functionality among its > > components. > > > > > >> > > > > > > > > > >> > > > > Finally, I hope I didn't convey that we are > > > > reverting/changing the > > > > > >> > > changes > > > > > >> > > > > made in KIP-412. The proposed changes would be an > addition > > > to > > > > it. > > > > > >> It > > > > > >> > > will > > > > > >> > > > > give brokers multiple ways of changing log levels. and > > there > > > > is > > > > > >> > still a > > > > > >> > > > > consistent way of achieving cross component goals of the > > > KIP. > > > > > >> > > > > > > > > > >> > > > > Best, > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > On Fri, Aug 23, 2019 at 4:12 PM Jason Gustafson < > > > > > >> ja...@confluent.io> > > > > > >> > > > > wrote: > > > > > >> > > > > > > > > > >> > > > > > Let me elaborate a little bit. We made the decision > > early > > > > on for > > > > > >> > > Connect > > > > > >> > > > > to > > > > > >> > > > > > use HTTP instead of Kafka's custom RPC protocol. In > > > > exchange for > > > > > >> > > losing > > > > > >> > > > > > some hygienic consistency with Kafka, we took easier > > > > integration > > > > > >> > with > > > > > >> > > > > > management tools. The scope of the connect REST APIs > is > > > > really > > > > > >> > > managing > > > > > >> > > > > the > > > > > >> > > > > > connect cluster. It has endpoints for creating > > connectors, > > > > > >> changing > > > > > >> > > > > > configs, seeing their health, etc. Doesn't debugging > fit > > > in > > > > with > > > > > >> > > that? I > > > > > >> > > > > am > > > > > >> > > > > > not sure I see why we would treat this as an > exceptional > > > > case. > > > > > >> > > > > > > > > > > >> > > > > > I personally see JMX as a necessary evil in Kafka > > because > > > > most > > > > > >> > > metrics > > > > > >> > > > > > agents have native support. But it is particularly > > painful > > > > when > > > > > >> it > > > > > >> > > comes > > > > > >> > > > > to > > > > > >> > > > > > use as an RPC mechanism. This was the central > motivation > > > > behind > > > > > >> > > KIP-412, > > > > > >> > > > > > which makes it very odd to see a new proposal which > > > suggests > > > > > >> > > > > standardizing > > > > > >> > > > > > on JMX for log level adjustment. I actually see this > as > > > > > >> something > > > > > >> > > we'd > > > > > >> > > > > want > > > > > >> > > > > > to eventually turn off in Kafka. Now that we have a > > proper > > > > API > > > > > >> with > > > > > >> > > > > support > > > > > >> > > > > > in the AdminClient, we can deprecate and eventually > > remove > > > > the > > > > > >> JMX > > > > > >> > > > > > endpoint. > > > > > >> > > > > > > > > > > >> > > > > > Thanks, > > > > > >> > > > > > Jason > > > > > >> > > > > > > > > > > >> > > > > > On Fri, Aug 23, 2019 at 10:49 AM Jason Gustafson < > > > > > >> > ja...@confluent.io > > > > > >> > > > > > > > > >> > > > > > wrote: > > > > > >> > > > > > > > > > > >> > > > > > > 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 > > > > > >> > > > > > >> > > > >> > > > > > > > > > >> > > > > > >> > > > >> > > > > > > > > >> > > > > > >> > > > >> > > > > > > > >> > > > > > >> > > > >> > > > > > > >> > > > > > >> > > > >> > > > > > >> > > > > > >> > > > > > > > > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > > >> > > > > > >> > > > > > > >> > > > > > >> > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > > > > > > > > > > >
