Thanks for the quick response, Tom, and thanks again for tweaking the
wording on KIP-676. We can absolutely revisit this in the future if it
becomes more of an issue.
If anyone else disagrees, please say so.
Best regards,
Randall
On Mon, Jan 25, 2021 at 9:19 AM Tom Bentley <tbent...@redhat.com> wrote:
> 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>*
> > > > >
> > > >
> > >
> >
>
