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