Hi Randall,
I agree that Kafka Connect's API is more usable given that the user of it
knows the semantics (and KIP-495 is clear on that point). So perhaps this
inconsistency isn't enough of a problem that it's worth fixing, at least
not at the moment.
Kind regards,
Tom
On Fri, Jan 22, 2021 at 6:36 PM Randall Hauch <rha...@gmail.com> wrote:
> Thanks for updating the wording in KIP-676.
>
> I guess the best path forward depends on what we think needs to change. If
> we think KIP-676 and the changes already made in AK 2.8 are not quite
> right, then maybe we should address this either by fixing the changes (and
> maybe updating KIP-676 as needed) or reverting the changes if bigger
> changes are necessary.
>
> OTOH, if we think KIP-676 and the changes already made in AK 2.8 are
> correct but we just need to update Connect to have similar behavior, then I
> don't see why we'd consider reverting the KIP-676 changes in AK 2.8. We
> could pass another KIP that amends the Connect dynamic logging REST API
> behavior and fix that in AK 3.0 (if there's not enough time for 2.8).
>
> However, it's not clear to me that the Connect dynamic logging REST API
> behavior is incorrect. The API only allows setting one level at a time
> (which is different than a Log4J configuration file), and so order matters.
> Consider this case:
> 1. PUT '{"level": "TRACE"}'
> http://localhost:8083/admin/loggers/org.apache.kafka.connect
> 2. PUT '{"level": "DEBUG"}'
> http://localhost:8083/admin/loggers/org.apache.kafka
>
> Is the second call intended to take precedence and override the first, or
> was the second not taking precedence over and instead augmenting the first?
> KIP-495 really considers the latest call to take precedence over all prior
> ones. This is simple, and ordering the updates can be used to get the
> desired behavior. For example, swapping the order of these calls easily
> gets the desired behavior of `org.apache.kafka.connect` (and descendants)
> are at a TRACE level.
>
> IIUC, you seem to suggest that step 2 should not override the fact that
> step 1 had already set the logger for `org.apache.kafka.connect`. In order
> to do this, we'd have to track all of the dynamic settings made since the
> VM started, support unsetting (deleting) previously-set levels, and take
> all of them into account when any changes are made to potentially apply the
> net effect of all dynamic settings across all logger contexts. Plus, we'd
> need the ability to list the set contexts just to know what could or should
> be deleted, and we'd have to remember the original state defined by the log
> config so that when dynamic logging context levels are deleted we can
> properly revert to the correct value (if not overridden by a higher-level
> dynamic context).
>
> In short, this dramatically increases the complexity of both the
> implementation and the UX behavior, and it's not clear whether all that
> complexity really adds much value. WDYT?
>
> Best regards,
>
> Randall
>
> On Fri, Jan 22, 2021 at 7:58 AM Tom Bentley <tbent...@redhat.com> wrote:
>
> > Hi Randall,
> >
> > Thanks for pointing this out. You're quite right about the behaviour of
> the
> > LoggingResource, and I've updated the KIP with your suggested wording.
> >
> > However, looking at it has made me realise that while KIP-676 means the
> > logger levels are now hierarchical there's still an inconsistency between
> > how levels are set in Kafka Connect and how it works in the broker.
> >
> > In log4j you can configure foo.baz=DEBUG and then foo=INFO and debug
> > messages from foo.baz.Y will continue to be logged because setting the
> > parent doesn't override all descendents (the level is inherited). As you
> > know, in Kafka Connect, the way the log setting works is to find all the
> > descendent loggers of foo and apply the given level to them, so setting
> > foo.baz=DEBUG and then foo=INFO means foo.baz.Y debug messages will not
> > appear.
> >
> > Obviously that behavior for Connect is explicitly stated in KIP-495, but
> I
> > can't help but feel that the KIP-676 changes not addressing this is a
> lost
> > opportunity.
> >
> > It's also worth bearing in mind that KIP-653[1] is (hopefully) going to
> > happen for Kafka 3.0.
> >
> > So I wonder if perhaps the pragmatic thing to do would be to:
> >
> > 1. Revert the changes for KIP-676 for Kafka 2.8
> > 2. Pass another KIP, to be implemented for Kafka 3.0, which makes all the
> > Kafka APIs consistent in both respecting the hierarchy and also in what
> > updating a logger level means.
> >
> > I don't have a particularly strong preference either way, but it seems
> > better, from a users PoV, if all these logging changes happened in a
> major
> > release and achieved consistency across components going forward.
> >
> > Thoughts?
> >
> > Kind regards,
> >
> > Tom
> >
> > [1]:
> >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-653%3A+Upgrade+log4j+to+log4j2
> >
> >
> >
> > On Thu, Jan 21, 2021 at 9:17 PM Randall Hauch <rha...@gmail.com> wrote:
> >
> > > Tom, et al.,
> > >
> > > I'm really late to the party and mistakenly thought the scope of this
> KIP
> > > included only the broker. But I now see in the KIP-676 [1] text the
> > > following claim:
> > >
> > > Kafka exposes a number of APIs for describing and changing logger
> levels:
> > > >
> > > > * The Kafka broker exposes the DescribeConfigs RPC with the
> > BROKER_LOGGER
> > > > config resource.
> > > > * Broker logger levels can also be configured using the
> > > > Log4jControllerMBean MBean, exposed through JMX as
> > > > kafka:type=kafka.Log4jController.
> > > > * Kafka Connect exposes the /admin/loggers REST API endpoint for
> > > > describing and changing logger levels.
> > > >
> > > > When accessing a logger's level these APIs do not respect the logger
> > > > hierarchy.
> > > > Instead, if the logger's level is not explicitly set the level of the
> > > root
> > > > logger is used, even when an intermediate logger is configured with a
> > > > different level.
> > > >
> > >
> > > Regarding Connect, the third bullet is accurate: Kafka
> > > Connect's `/admin/loggers/` REST API introduced via KIP-495 [2] does
> > > describe and change logger levels.
> > >
> > > But the first sentence after the bullets is inaccurate, because per
> > KIP-495
> > > the Kafka Connect `/admin/loggers/` REST API does respect the
> hierarchy.
> > In
> > > fact, this case is explicitly called out in KIP-495:
> > >
> > > Setting the log level of an ancestor (for example,
> > > > `org.apache.kafka.connect` as opposed to a classname) will update the
> > > > levels of all child classes.
> > > >
> > >
> > > and there are even unit tests in Connect for that case [3].
> > >
> > > Can we modify this sentence in KIP-676 to reflect this? One way to
> > minimize
> > > the changes to the already-approved KIP is to change:
> > >
> > > > When accessing a logger's level these APIs do not respect the logger
> > > > hierarchy.
> > >
> > > to
> > >
> > > > When accessing a logger's level the first two of these APIs do not
> > > respect
> > > > the logger hierarchy.
> > >
> > >
> > > Thoughts?
> > >
> > > Randall
> > >
> > > [1]
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-676%3A+Respect+logging+hierarchy
> > > [2]
> > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-495%3A+Dynamically+Adjust+Log+Levels+in+Connect
> > > [3]
> > >
> > >
> >
> https://github.com/apache/kafka/blob/trunk/connect/runtime/src/test/java/org/apache/kafka/connect/runtime/rest/resources/LoggingResourceTest.java#L118-L124
> > >
> > > On Thu, Oct 8, 2020 at 9:56 AM Dongjin Lee <dong...@apache.org> wrote:
> > >
> > > > Hi Tom,
> > > >
> > > > I also agree that the current behavior is clearly wrong and I think
> it
> > > was
> > > > mistakenly omitted in the KIP-412 discussion process. The current
> > > > implementation does not reflect log4j's logger hierarchy.
> > > >
> > > > Regards,
> > > > Dongjin
> > > >
> > > > On Thu, Oct 8, 2020 at 1:27 AM John Roesler <vvcep...@apache.org>
> > wrote:
> > > >
> > > > > Ah, thanks Tom,
> > > > >
> > > > > My only concern was that we might silently start logging a
> > > > > lot more or less after the upgrade, but if the logging
> > > > > behavior won't change at all, then the concern is moot.
> > > > >
> > > > > Since the KIP is only to make the APIs return an accurrate
> > > > > representation of the actual log level, I have no concerns
> > > > > at all.
> > > > >
> > > > > Thanks,
> > > > > -John
> > > > >
> > > > > On Wed, 2020-10-07 at 17:00 +0100, Tom Bentley wrote:
> > > > > > Hi John,
> > > > > >
> > > > > > You're right, but note that this affects the level the
> > broker/connect
> > > > > > worker was _reporting_ for that logger, not the level at which
> the
> > > > logger
> > > > > > was actually logging, which would be TRACE both before and after
> > > > > upgrading.
> > > > > >
> > > > > > I've added more of an explanation to the KIP, since it wasn't
> very
> > > > clear.
> > > > > >
> > > > > > Thanks for taking a look.
> > > > > >
> > > > > > Tom
> > > > > >
> > > > > > On Wed, Oct 7, 2020 at 4:29 PM John Roesler <vvcep...@apache.org
> >
> > > > wrote:
> > > > > >
> > > > > > > Thanks for this KIP Tom,
> > > > > > >
> > > > > > > Just to clarify the impact: In your KIP you described a
> > > > > > > situation in which the root logger is configured at INFO, an
> > > > > > > "kafka.foo" is configured at TRACE, and then "kafka.foo.bar"
> > > > > > > is resolved to INFO.
> > > > > > >
> > > > > > > Assuming this goes into 3.0, would it be the case that if I
> > > > > > > had the above configuration, after upgrade, "kafka.foo.bar"
> > > > > > > would just switch from INFO to TRACE on its own?
> > > > > > >
> > > > > > > It seems like it must, since it's not configured explicitly,
> > > > > > > and we are changing the inheritance rule from "inherit
> > > > > > > directly from root" to "inherit from the closest configured
> > > > > > > ancestor in the hierarchy".
> > > > > > >
> > > > > > > Am I thinking about this right?
> > > > > > >
> > > > > > > Thanks,
> > > > > > > -John
> > > > > > >
> > > > > > > On Wed, 2020-10-07 at 15:42 +0100, Tom Bentley wrote:
> > > > > > > > Hi all,
> > > > > > > >
> > > > > > > > I would like to start discussion on a small KIP which seeks
> to
> > > > > rectify an
> > > > > > > > inconsistency between how Kafka reports logger levels and how
> > > > logger
> > > > > > > > configuration is inherited hierarchically in log4j.
> > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-676%3A+Respect+logging+hierarchy
> > > > > > > > <
> > > > > > >
> > > > >
> > > >
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-676%3A+Respect+logging+hierarchy?moved=true
> > > > > > > >
> > > > > > > > If you have a few minutes to have a look I'd be grateful for
> > any
> > > > > > > feedback.
> > > > > > > > Many thanks,
> > > > > > > >
> > > > > > > > Tom
> > > > >
> > > > >
> > > >
> > > > --
> > > > *Dongjin Lee*
> > > >
> > > > *A hitchhiker in the mathematical world.*
> > > >
> > > >
> > > >
> > > >
> > > > *github: <http://goog_969573159/>github.com/dongjinleekr
> > > > <https://github.com/dongjinleekr>keybase:
> > > https://keybase.io/dongjinleekr
> > > > <https://keybase.io/dongjinleekr>linkedin:
> > > kr.linkedin.com/in/dongjinleekr
> > > > <https://kr.linkedin.com/in/dongjinleekr>speakerdeck:
> > > > speakerdeck.com/dongjin
> > > > <https://speakerdeck.com/dongjin>*
> > > >
> > >
> >
>
