Looks great. Thanks again, Arjun! On Tue, Sep 17, 2019 at 1:17 AM Arjun Satish <arjun.sat...@gmail.com> wrote:
> Answers inline > > On Mon, Sep 16, 2019 at 5:06 PM Randall Hauch <rha...@gmail.com> wrote: > > > 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. > > > > The GET methods should return all initialized loggers (ancestors and the > ones created by runtime classes). > > > > > > 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. > > > > The ancestors are also specified in the GET responses. Updated the KIP to > highlight this. > > > > > > 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 > > > > > > > >> > > > > > >> > > > >> > > > > > > > > > > > >> > > > > > >> > > > >> > > > > > > > > > > >> > > > > > >> > > > >> > > > > > > > > > >> > > > > > >> > > > >> > > > > > > > > >> > > > > > >> > > > >> > > > > > > > >> > > > > > >> > > > > > > > > > > > >> > > > > > >> > > > > > > > > > > >> > > > > > >> > > > > > > > > > >> > > > > > >> > > > > > > > > >> > > > > > >> > > > > > > > >> > > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
