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