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