config test improve

kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigBuilder.scala

@@ -133,7 +133,7 @@ private[kyuubi] case class TypedConfigBuilder[T](
   }
 
   def createOptional: OptionalConfigEntry[T] = {
-    val entry = OptionalConfigEntry(parent.key, fromStr, toStr, parent._doc, parent._version)
+    val entry = new OptionalConfigEntry(parent.key, fromStr, toStr, parent._doc, parent._version)
     parent._onCreate.foreach(_(entry))
     entry
   }
@@ -143,13 +143,13 @@ private[kyuubi] case class TypedConfigBuilder[T](
     case _ =>
       val d = fromStr(toStr(default))
       val entry =
-        ConfigEntryWithDefault(parent.key, d, fromStr, toStr, parent._doc, parent._version)
+        new ConfigEntryWithDefault(parent.key, d, fromStr, toStr, parent._doc, parent._version)
       parent._onCreate.foreach(_(entry))
       entry
   }
 
   def createWithDefaultString(default: String): ConfigEntryWithDefaultString[T] = {
-    val entry = ConfigEntryWithDefaultString(
+    val entry = new ConfigEntryWithDefaultString(
       parent.key, default, fromStr, toStr, parent._doc, parent._version)
     parent._onCreate.foreach(_(entry))
     entry

kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigEntry.scala

@@ -25,9 +25,9 @@ trait ConfigEntry[T] {
   def version: String
 
   def defaultValStr: String
-  def defaultVal: Option[T] = None
+  def defaultVal: Option[T]
 
-  final override def toString: String = {
+  override def toString: String = {
     s"ConfigEntry(key=$key, defaultValue=$defaultValStr, doc=$doc, version=$version)"
   }
 
@@ -40,30 +40,44 @@ trait ConfigEntry[T] {
   ConfigEntry.registerEntry(this)
 }
 
-case class OptionalConfigEntry[T](
-    key: String,
+class OptionalConfigEntry[T](
+    _key: String,
     rawValueConverter: String => T,
     rawStrConverter: T => String,
-    doc: String,
-    version: String) extends ConfigEntry[Option[T]] {
-  override def valueConverter: String => Option[T] = s => Option(rawValueConverter(s))
+    _doc: String,
+    _version: String) extends ConfigEntry[Option[T]] {
+  override def valueConverter: String => Option[T] = {
+    s => Option(rawValueConverter(s))
+  }
 
-  override def strConverter: Option[T] => String = v => v.map(rawStrConverter).orNull
+  override def strConverter: Option[T] => String = {
+    v => v.map(rawStrConverter).orNull
+  }
 
-  override def defaultValStr: String = ConfigEntry.UNDEFINED
+  override def defaultValStr: String = {
+    ConfigEntry.UNDEFINED
+  }
 
   override def readFrom(conf: ConfigProvider): Option[T] = {
     readString(conf).map(rawValueConverter)
   }
+
+  override def defaultVal: Option[Option[T]] = None
+
+  override def key: String = _key
+
+  override def doc: String = _doc
+
+  override def version: String = _version
 }
 
-case class ConfigEntryWithDefault[T](
-    key: String,
+class ConfigEntryWithDefault[T](
+    _key: String,
     _defaultVal: T,
-    valueConverter: String => T,
-    strConverter: T => String,
-    doc: String,
-    version: String) extends ConfigEntry[T] {
+    _valueConverter: String => T,
+    _strConverter: T => String,
+    _doc: String,
+    _version: String) extends ConfigEntry[T] {
   override def defaultValStr: String = strConverter(_defaultVal)
 
   override def defaultVal: Option[T] = Option(_defaultVal)
@@ -71,15 +85,25 @@ case class ConfigEntryWithDefault[T](
   override def readFrom(conf: ConfigProvider): T = {
     readString(conf).map(valueConverter).getOrElse(_defaultVal)
   }
+
+  override def key: String = _key
+
+  override def valueConverter: String => T = _valueConverter
+
+  override def strConverter: T => String = _strConverter
+
+  override def doc: String = _doc
+
+  override def version: String = _version
 }
 
-case class ConfigEntryWithDefaultString[T](
-   key: String,
+class ConfigEntryWithDefaultString[T](
+   _key: String,
    _defaultVal: String,
-   valueConverter: String => T,
-   strConverter: T => String,
-   doc: String,
-   version: String) extends ConfigEntry[T] {
+   _valueConverter: String => T,
+   _strConverter: T => String,
+   _doc: String,
+   _version: String) extends ConfigEntry[T] {
   override def defaultValStr: String = _defaultVal
 
   override def defaultVal: Option[T] = Some(valueConverter(_defaultVal))
@@ -88,6 +112,16 @@ case class ConfigEntryWithDefaultString[T](
     val value = readString(conf).getOrElse(_defaultVal)
     valueConverter(value)
   }
+
+  override def key: String = _key
+
+  override def valueConverter: String => T = _valueConverter
+
+  override def strConverter: T => String = _strConverter
+
+  override def doc: String = _doc
+
+  override def version: String = _version
 }
 
 object ConfigEntry {
@@ -99,6 +133,4 @@ object ConfigEntry {
     val existing = knownConfigs.putIfAbsent(entry.key, entry)
     require(existing == null, s"Config entry ${entry.key} already registered!")
   }
-
-  def findEntry(key: String): ConfigEntry[_] = knownConfigs.get(key)
 }

kyuubi-common/src/main/scala/org/apache/kyuubi/config/ConfigHelpers.scala

@@ -28,13 +28,4 @@ object ConfigHelpers {
   def seqToStr[T](v: Seq[T], stringConverter: T => String): String = {
     v.map(stringConverter).mkString(",")
   }
-
-  def toNumber[T](s: String, converter: String => T, key: String, configType: String): T = {
-    try {
-      converter(s.trim)
-    } catch {
-      case _: NumberFormatException =>
-        throw new IllegalArgumentException(s"$key should be $configType, but was $s")
-    }
-  }
 }

kyuubi-common/src/main/scala/org/apache/kyuubi/config/KyuubiConf.scala

@@ -51,7 +51,7 @@ case class KyuubiConf(loadSysDefault: Boolean = true) extends Logging {
   }
 
   def set[T](entry: OptionalConfigEntry[T], value: T): KyuubiConf = {
-    set(entry.key, entry.rawStrConverter(value))
+    set(entry.key, entry.strConverter(Option(value)))
     this
   }
 

kyuubi-common/src/test/scala/org/apache/kyuubi/config/ConfigEntrySuite.scala

@@ -0,0 +1,73 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.config
+
+import org.apache.kyuubi.KyuubiFunSuite
+
+class ConfigEntrySuite extends KyuubiFunSuite {
+
+  test("optional config entry") {
+    val e1 = new OptionalConfigEntry[Int](
+      "kyuubi.int.spark",
+      s => s.toInt + 1,
+      v => (v - 1).toString,
+      "this is dummy documentation",
+      "<none>")
+
+    val conf = KyuubiConf()
+    assert(conf.get(e1).isEmpty)
+    val e = intercept[IllegalArgumentException](new OptionalConfigEntry[Int](
+      "kyuubi.int.spark",
+      s => s.toInt + 1,
+      v => (v - 1).toString,
+      "this is dummy documentation",
+      "<none>"))
+    assert(e.getMessage ===
+      "requirement failed: Config entry kyuubi.int.spark already registered!")
+    conf.set(e1.key, "2")
+    assert(conf.get(e1).get === 3)
+
+  }
+
+  test("config entry with default") {
+    val e1 = new ConfigEntryWithDefault[Long]("kyuubi.long.spark",
+      2,
+      s => s.toLong + 1,
+      v => (v - 1).toString,
+    "",
+    "")
+    val conf = KyuubiConf()
+    assert(conf.get(e1) === 2)
+    conf.set(e1.key, "5")
+    assert(conf.get(e1) === 6)
+  }
+
+  test("config entry with default string") {
+    val e1 = new ConfigEntryWithDefaultString[Double](
+      "kyuubi.double.spark",
+      "3.0", s => java.lang.Double.valueOf(s),
+      v => v.toString,
+      "",
+      "")
+    val conf = KyuubiConf()
+    assert(conf.get(e1) === 3.0)
+    conf.set(e1.asInstanceOf[ConfigEntry[AnyVal]], 5.0)
+    assert(conf.get(e1) === 5.0)
+  }
+
+}

kyuubi-common/src/test/scala/org/apache/kyuubi/config/ConfigProviderSuite.scala

@@ -0,0 +1,35 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *    http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.kyuubi.config
+
+import scala.collection.JavaConverters._
+
+import org.apache.kyuubi.KyuubiFunSuite
+
+class ConfigProviderSuite extends KyuubiFunSuite {
+
+  test("config provider") {
+    val conf = Map("kyuubi.abc" -> "1", "kyuubi.xyz" -> "2", "spark.abc" -> "kyuubi")
+    val provider = new ConfigProvider(conf.asJava)
+    assert(provider.get("kyuubi.abc").get === "1")
+    assert(provider.get("kyuubi.xyz").get === "2")
+    assert(provider.get("spark.abc") === None)
+
+  }
+
+}