You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by GitBox <gi...@apache.org> on 2020/09/08 16:18:40 UTC
[GitHub] [kafka] tombentley opened a new pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
tombentley opened a new pull request #9266:
URL: https://github.com/apache/kafka/pull/9266
Previous to root logger level was used, ignoring intervening loggers with different levels.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-704971958
Thanks for the update @ijuma, I'll prepare a KIP for 3.0.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-690954007
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696170285
Build timed out for some reason, started another one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r485671503
##########
File path: core/src/test/scala/kafka/utils/LoggingTest.scala
##########
@@ -58,4 +60,20 @@ class LoggingTest extends Logging {
assertEquals(logging.getClass.getName, logging.log.underlying.getName)
}
+
+ @Test
+ def testLoggerLevelIsResolved(): Unit = {
+ val controller = new Log4jController()
+ val previousLevel = controller.getLogLevel("kafka")
+ try {
+ controller.setLogLevel("kafka", "TRACE")
Review comment:
@dongjinleekr that would be fine for the `setLogLevel()` and `assertEquals` calls, but there would still be the level name in the `assertTrue(...contains("kafka=TRACE"))` calls. I think fixing it there would make the code harder to read, and given that the declaration of `TRACE` is in an external API on which we depend, and the name will not change there doesn't seem to be any benefit to using the declaration rather than the name as a literal.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-694255418
Many thanks @ijuma all fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696622982
@ijuma I've fixed an infinite loop, could you trigger the build again pleast?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r495975325
##########
File path: core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
##########
@@ -2061,11 +2062,12 @@ class PlaintextAdminIntegrationTest extends BaseAdminIntegrationTest {
@Test
def testDescribeConfigsForLog4jLogLevels(): Unit = {
client = Admin.create(createConfig)
-
+ LoggerFactory.getLogger("kafka.cluster.Replica").trace("Message to create the logger")
val loggerConfig = describeBrokerLoggers()
- val rootLogLevel = loggerConfig.get(Log4jController.ROOT_LOGGER).value()
+ val kafkaLogLevel = loggerConfig.get("kafka").value()
val logCleanerLogLevelConfig = loggerConfig.get("kafka.cluster.Replica")
- assertEquals(rootLogLevel, logCleanerLogLevelConfig.value()) // we expect an undefined log level to be the same as the root logger
+ // we expect an undefined log level to be the same as its first ancestor logger
Review comment:
Wouldn't the first ancestor be `kafka.cluster`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-724734814
I have reapplied the change https://github.com/apache/kafka/commit/960e64072592bc1ad939518b58dbdfb17068680d
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r490260948
##########
File path: core/src/test/scala/kafka/utils/LoggingTest.scala
##########
@@ -58,4 +60,22 @@ class LoggingTest extends Logging {
assertEquals(logging.getClass.getName, logging.log.underlying.getName)
}
+
+ @Test
+ def testLoggerLevelIsResolved(): Unit = {
+ val controller = new Log4jController()
+ val previousLevel = controller.getLogLevel("kafka")
+ try {
+ controller.setLogLevel("kafka", "TRACE")
+ // Do some logging so that the Logger is created within the hierarchy
+ // (until loggers are used only loggers in the config file exist)
+ Logger(LoggerFactory.getLogger("kafka.utils.Log4jControllerTest")).trace("test")
+ Assert.assertEquals("TRACE", controller.getLogLevel("kafka"))
+ Assert.assertEquals("TRACE", controller.getLogLevel("kafka.utils.Log4jControllerTest"))
+ Assert.assertTrue(controller.getLoggers.contains("kafka=TRACE"))
+ Assert.assertTrue(controller.getLoggers.contains("kafka.utils.Log4jControllerTest=TRACE"))
Review comment:
Nit: we don't usually include the `Assert.` prefix in test assertions.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-694377628
@dongjinleekr done.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley edited a comment on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley edited a comment on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696622982
@ijuma I've fixed an infinite loop, could you trigger the build again please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r485618779
##########
File path: core/src/test/scala/kafka/utils/LoggingTest.scala
##########
@@ -58,4 +60,20 @@ class LoggingTest extends Logging {
assertEquals(logging.getClass.getName, logging.log.underlying.getName)
}
+
+ @Test
+ def testLoggerLevelIsResolved(): Unit = {
+ val controller = new Log4jController()
+ val previousLevel = controller.getLogLevel("kafka")
+ try {
+ controller.setLogLevel("kafka", "TRACE")
Review comment:
@tombentley A small propsal: would `org.apache.log4j.Level.TRACE.toString` be better?
It passes compilation checks.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-688987558
@ijuma please could you take a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-721228240
Maybe @vvcephei, @mimaison or @gwenshap could take a look at this since you voted on the KIP? Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-698835897
@ijuma any chance you could trigger a CI build? Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-697220456
@ijuma sorry about that. Now fixed.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r490259458
##########
File path: core/src/main/scala/kafka/utils/Log4jController.scala
##########
@@ -29,6 +29,24 @@ import scala.jdk.CollectionConverters._
object Log4jController {
val ROOT_LOGGER = "root"
+ def resolveLevel(logger: Logger): String = {
Review comment:
Should this be private?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r485673100
##########
File path: core/src/test/scala/kafka/utils/LoggingTest.scala
##########
@@ -58,4 +60,20 @@ class LoggingTest extends Logging {
assertEquals(logging.getClass.getName, logging.log.underlying.getName)
}
+
+ @Test
+ def testLoggerLevelIsResolved(): Unit = {
+ val controller = new Log4jController()
+ val previousLevel = controller.getLogLevel("kafka")
+ try {
+ controller.setLogLevel("kafka", "TRACE")
Review comment:
@tombentley Great. Your stance is reasonable. I am convinced.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] stanislavkozlovski commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
stanislavkozlovski commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-689043684
Sorry, I mistook `kafka` for the root logger. I agree it tests the same path
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r490256227
##########
File path: core/src/test/scala/kafka/utils/LoggingTest.scala
##########
@@ -58,4 +60,20 @@ class LoggingTest extends Logging {
assertEquals(logging.getClass.getName, logging.log.underlying.getName)
}
+
+ @Test
+ def testLoggerLevelIsResolved(): Unit = {
+ val controller = new Log4jController()
+ val previousLevel = controller.getLogLevel("kafka")
+ try {
+ controller.setLogLevel("kafka", "TRACE")
+ Logger(LoggerFactory.getLogger("kafka.utils.Log4jControllerTest")).trace("test")
Review comment:
Can we add a comment explaining why we are invoking the logger here?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-714458486
@ijuma [KIP-676](https://cwiki.apache.org/confluence/display/KAFKA/KIP-676%3A+Respect+logging+hierarchy?moved=true) has been approved, so I think we should be able to merge this change to trunk.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696622982
@ijuma I've fixed an infinite loop, could you trigger the build again pleast?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-721231570
Great, thanks Ismael.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696784359
@tombentley `testDescribeConfigsForLog4jLogLevels` is failing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-701532641
@ijuma I initially saw it as a bug, because it's pretty surprising behaviour (because it differs from how logger inheritance works in log4j), but I hadn't realised until after you merged that it was specified in a KIP. KIP freeze for 2.7 is today, so there's no time to get a KIP approved for this in the 2.7 timeframe. I guess that means we'll have to revert it for now and I write a KIP to target 3.0. Sorry about this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-721229160
We just need to revert 75e5358 (which reverted the original merged). Do you need an new PR for that?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-704966543
@tombentley Thanks for the explanation. I have reverted this change for now: https://github.com/apache/kafka/commit/75e53585255a0c2737dee84ddbad39094c5e8500
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-698835897
@ijuma any chance you could trigger a CI build? Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-724744145
Thanks Ismael.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-690954007
@ijuma @hachikuji @guozhangwang any chance one of you could review this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-690954007
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696784359
@tombentley `testDescribeConfigsForLog4jLogLevels` is failing.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r496048334
##########
File path: core/src/test/scala/integration/kafka/api/PlaintextAdminIntegrationTest.scala
##########
@@ -2061,11 +2062,12 @@ class PlaintextAdminIntegrationTest extends BaseAdminIntegrationTest {
@Test
def testDescribeConfigsForLog4jLogLevels(): Unit = {
client = Admin.create(createConfig)
-
+ LoggerFactory.getLogger("kafka.cluster.Replica").trace("Message to create the logger")
val loggerConfig = describeBrokerLoggers()
- val rootLogLevel = loggerConfig.get(Log4jController.ROOT_LOGGER).value()
+ val kafkaLogLevel = loggerConfig.get("kafka").value()
val logCleanerLogLevelConfig = loggerConfig.get("kafka.cluster.Replica")
- assertEquals(rootLogLevel, logCleanerLogLevelConfig.value()) // we expect an undefined log level to be the same as the root logger
+ // we expect an undefined log level to be the same as its first ancestor logger
Review comment:
It would, but that's not configured in the `log4j.properties` used for this test, so we inherit from the 2nd ancestor, which is ERROR, where as the root logger was OFF. Which is why I had to fix this test. Let me fix the comment.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma merged pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma merged pull request #9266:
URL: https://github.com/apache/kafka/pull/9266
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-701445114
@ijuma it's just been brought to my attention that the root looger default behaviour is actually explictly specified in [KIP-412](https://cwiki.apache.org/confluence/display/KAFKA/KIP-412%3A+Extend+Admin+API+to+support+dynamic+application+log+levels#KIP412:ExtendAdminAPItosupportdynamicapplicationloglevels-DescribeConfigs) (search for "Returns the root logger's log level if this logger is not set explicitly yet"). I hadn't previously realised that this was specified in a KIP. So is it OK to do this without a KIP?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-701518513
I thought this was a bug versus changing the spec. If we are changing the spec, then we do need a KIP.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696170285
Build timed out for some reason, started another one.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley edited a comment on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley edited a comment on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-696622982
@ijuma I've fixed an infinite loop, could you trigger the build again please?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on a change in pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
dongjinleekr commented on a change in pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#discussion_r490383726
##########
File path: core/src/test/scala/kafka/utils/LoggingTest.scala
##########
@@ -58,4 +60,20 @@ class LoggingTest extends Logging {
assertEquals(logging.getClass.getName, logging.log.underlying.getName)
}
+
+ @Test
+ def testLoggerLevelIsResolved(): Unit = {
+ val controller = new Log4jController()
+ val previousLevel = controller.getLogLevel("kafka")
+ try {
+ controller.setLogLevel("kafka", "TRACE")
+ Logger(LoggerFactory.getLogger("kafka.utils.Log4jControllerTest")).trace("test")
Review comment:
I verified that it works without `com.typesafe.scalalogging.Logger#apply` here, i.e., `LoggerFactory.getLogger("kafka.utils.Log4jControllerTest").trace("test")`. Other test cases like `RestServerTest` don't invoke it. It would be better to remove it.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] ijuma commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
ijuma commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-721229952
I can reapply the change soon.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-693952069
@ijuma @hachikuji @guozhangwang @mjsax @mimaison please could one of you could take a look?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] tombentley commented on pull request #9266: KAFKA-10469: Resolve logger levels hierarchically
Posted by GitBox <gi...@apache.org>.
tombentley commented on pull request #9266:
URL: https://github.com/apache/kafka/pull/9266#issuecomment-689020104
@stanislavkozlovski I'm not sure how what you describe differs from the test implemented, though it's not identically the same because the tests have their own `log4j.properties` which configures `kafka`, but not `kafka.controller`. I guess it might be clearer (and less fragile) if the test loggers were `foo` and `foo.bar`.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org