From c538ba8f3121d637b464de1ebf085a3b1b41e1f2 Mon Sep 17 00:00:00 2001 From: Ismael Juma Date: Sat, 3 Aug 2019 10:59:09 -0700 Subject: [PATCH 1/3] MINOR: Fix potential bug in LogConfig.getConfigValue and improve test coverage LogConfig.getConfigValue would throw a NoSuchElementException if any log config was defined without a server default mapping. Added a unit test for `getConfigValue` and a sanity test for `toHtml`/`toRst`/`toEnrichedRst`, which were previously not exercised during the test suite. --- core/src/main/scala/kafka/log/LogConfig.scala | 18 ++++++-- .../scala/unit/kafka/log/LogConfigTest.scala | 46 ++++++++++++++++++- 2 files changed, 59 insertions(+), 5 deletions(-) diff --git a/core/src/main/scala/kafka/log/LogConfig.scala b/core/src/main/scala/kafka/log/LogConfig.scala index bcc3f1208f397..5e0917bc84a2c 100755 --- a/core/src/main/scala/kafka/log/LogConfig.scala +++ b/core/src/main/scala/kafka/log/LogConfig.scala @@ -181,9 +181,17 @@ object LogConfig { "[PartitionId]:[BrokerId],[PartitionId]:[BrokerId]:... or alternatively the wildcard '*' can be used to throttle " + "all replicas for this topic." - private class LogConfigDef extends ConfigDef { + private[log] val ServerDefaultHeaderName = "Server Default Property" + + // Package private for testing + private[log] class LogConfigDef(base: ConfigDef) extends ConfigDef(base) { + def this() = this(new ConfigDef) private final val serverDefaultConfigNames = mutable.Map[String, String]() + base match { + case b: LogConfigDef => serverDefaultConfigNames ++= b.serverDefaultConfigNames + case _ => + } def define(name: String, defType: ConfigDef.Type, defaultValue: Any, validator: Validator, importance: ConfigDef.Importance, doc: String, serverDefaultConfigName: String): LogConfigDef = { @@ -206,11 +214,12 @@ object LogConfig { this } - override def headers = List("Name", "Description", "Type", "Default", "Valid Values", "Server Default Property", "Importance").asJava + override def headers = List("Name", "Description", "Type", "Default", "Valid Values", ServerDefaultHeaderName, + "Importance").asJava override def getConfigValue(key: ConfigKey, headerName: String): String = { headerName match { - case "Server Default Property" => serverDefaultConfigNames.get(key.name).get + case ServerDefaultHeaderName => serverDefaultConfigNames.getOrElse(key.name, null) case _ => super.getConfigValue(key, headerName) } } @@ -218,6 +227,9 @@ object LogConfig { def serverConfigName(configName: String): Option[String] = serverDefaultConfigNames.get(configName) } + // Package private for testing, return a copy since it's a global variable + private[log] def configDefCopy: LogConfigDef = new LogConfigDef(configDef) + private val configDef: LogConfigDef = { import org.apache.kafka.common.config.ConfigDef.Importance._ import org.apache.kafka.common.config.ConfigDef.Range._ diff --git a/core/src/test/scala/unit/kafka/log/LogConfigTest.scala b/core/src/test/scala/unit/kafka/log/LogConfigTest.scala index 2cd79041fb479..6f3332f98350c 100644 --- a/core/src/test/scala/unit/kafka/log/LogConfigTest.scala +++ b/core/src/test/scala/unit/kafka/log/LogConfigTest.scala @@ -19,9 +19,11 @@ package kafka.log import java.util.Properties -import kafka.server.{ThrottledReplicaListValidator, KafkaConfig, KafkaServer} +import kafka.server.{KafkaConfig, KafkaServer, ThrottledReplicaListValidator} import kafka.utils.TestUtils -import org.apache.kafka.common.config.ConfigException +import org.apache.kafka.common.config.ConfigDef.Importance.MEDIUM +import org.apache.kafka.common.config.ConfigDef.Type.INT +import org.apache.kafka.common.config.{ConfigException, TopicConfig} import org.junit.{Assert, Test} import org.junit.Assert._ import org.scalatest.Assertions._ @@ -113,6 +115,46 @@ class LogConfigTest { assertFalse(isValid("100:0,10 : ")) } + /* Sanity check that toHtml produces one of the expected configs */ + @Test + def testToHtml(): Unit = { + val html = LogConfig.configDefCopy.toHtmlTable + val expectedConfig = "file.delete.delay.ms" + assertTrue(s"Could not find `$expectedConfig` in:\n $html", html.contains(expectedConfig)) + } + + /* Sanity check that toEnrichedRst produces one of the expected configs */ + @Test + def testToEnrichedRst(): Unit = { + val rst = LogConfig.configDefCopy.toEnrichedRst + val expectedConfig = "``file.delete.delay.ms``" + assertTrue(s"Could not find `$expectedConfig` in:\n $rst", rst.contains(expectedConfig)) + } + + /* Sanity check that toEnrichedRst produces one of the expected configs */ + @Test + def testToRst(): Unit = { + val rst = LogConfig.configDefCopy.toRst + val expectedConfig = "``file.delete.delay.ms``" + assertTrue(s"Could not find `$expectedConfig` in:\n $rst", rst.contains(expectedConfig)) + } + + @Test + def testGetConfigValue(): Unit = { + // Add a config that doesn't set the `serverDefaultConfigName` + val configDef = LogConfig.configDefCopy + val configNameWithNoServerMapping = "log.foo" + configDef.define(configNameWithNoServerMapping, INT, 1, MEDIUM, s"$configNameWithNoServerMapping doc") + + val deleteDelayKey = configDef.configKeys.get(TopicConfig.FILE_DELETE_DELAY_MS_CONFIG) + val deleteDelayServerDefault = configDef.getConfigValue(deleteDelayKey, LogConfig.ServerDefaultHeaderName) + assertEquals(KafkaConfig.LogDeleteDelayMsProp, deleteDelayServerDefault) + + val keyWithNoServerMapping = configDef.configKeys.get(configNameWithNoServerMapping) + val nullServerDefault = configDef.getConfigValue(keyWithNoServerMapping, LogConfig.ServerDefaultHeaderName) + assertNull(nullServerDefault) + } + private def isValid(configValue: String): Boolean = { try { ThrottledReplicaListValidator.ensureValidString("", configValue) From c9eebbccc1a9ee02d788e8673bb9682cf75916c6 Mon Sep 17 00:00:00 2001 From: Ismael Juma Date: Sat, 3 Aug 2019 10:59:31 -0700 Subject: [PATCH 2/3] Remove extra whitespace --- .../java/org/apache/kafka/clients/producer/ProducerConfig.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java index 4a4aebaf3248d..0d87284c1cf50 100644 --- a/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java +++ b/clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java @@ -405,7 +405,7 @@ public static Set configNames() { } public static ConfigDef configDef() { - return new ConfigDef(CONFIG); + return new ConfigDef(CONFIG); } public static void main(String[] args) { From 34f344cf050af9f0267f50ab9456d7ab541afef1 Mon Sep 17 00:00:00 2001 From: Ismael Juma Date: Sat, 3 Aug 2019 11:01:56 -0700 Subject: [PATCH 3/3] Tweak comment --- core/src/main/scala/kafka/log/LogConfig.scala | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/scala/kafka/log/LogConfig.scala b/core/src/main/scala/kafka/log/LogConfig.scala index 5e0917bc84a2c..c61e023a3682f 100755 --- a/core/src/main/scala/kafka/log/LogConfig.scala +++ b/core/src/main/scala/kafka/log/LogConfig.scala @@ -227,7 +227,7 @@ object LogConfig { def serverConfigName(configName: String): Option[String] = serverDefaultConfigNames.get(configName) } - // Package private for testing, return a copy since it's a global variable + // Package private for testing, return a copy since it's a mutable global variable private[log] def configDefCopy: LogConfigDef = new LogConfigDef(configDef) private val configDef: LogConfigDef = {