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