From 56287dba57dcd5bb3087b8fcb26a7ab918d52618 Mon Sep 17 00:00:00 2001 From: Ryan Williams Date: Thu, 12 Feb 2015 19:06:25 +0000 Subject: [PATCH 1/7] throw if nonexistent metrics config file provided previous behavior was to log an error; this is fine in the general case where no `spark.metrics.conf` parameter was specified, in which case a default `metrics.properties` is looked for, and the execption logged and suppressed if it doesn't exist. if the user has purposefully specified a metrics.conf file, however, it makes more sense to show them an error when said file doesn't exist. --- .../apache/spark/metrics/MetricsConfig.scala | 30 +++++++++++-------- 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala index 1b7a5d1f1980a..e4d219c6516ce 100644 --- a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala +++ b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala @@ -47,21 +47,27 @@ private[spark] class MetricsConfig(val configFile: Option[String]) extends Loggi setDefaultProperties(properties) // If spark.metrics.conf is not set, try to get file in class path - var is: InputStream = null - try { - is = configFile match { - case Some(f) => new FileInputStream(f) - case None => Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF) + (configFile match { + case Some(f) => { + logInfo(s"Loading MetricsConfig file: $f") + Some(new FileInputStream(f)) } - - if (is != null) { + case None => + try { + Some(Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF)) + } catch { + case e: Exception => { + logError("Error loading default configuration file", e) + None + } + } + }).foreach(is => + try { properties.load(is) + } finally { + is.close() } - } catch { - case e: Exception => logError("Error loading configure file", e) - } finally { - if (is != null) is.close() - } + ) propertyCategories = subProperties(properties, INSTANCE_REGEX) if (propertyCategories.contains(DEFAULT_PREFIX)) { From 31d2c3057b07d468cd0d9e71f983341b14a2521c Mon Sep 17 00:00:00 2001 From: Ryan Williams Date: Thu, 12 Feb 2015 20:47:20 +0000 Subject: [PATCH 2/7] metrics code review feedback --- .../apache/spark/metrics/MetricsConfig.scala | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala index e4d219c6516ce..e8b017016d85e 100644 --- a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala +++ b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala @@ -47,27 +47,23 @@ private[spark] class MetricsConfig(val configFile: Option[String]) extends Loggi setDefaultProperties(properties) // If spark.metrics.conf is not set, try to get file in class path - (configFile match { - case Some(f) => { - logInfo(s"Loading MetricsConfig file: $f") - Some(new FileInputStream(f)) + val isOpt: Option[InputStream] = configFile.map(new FileInputStream(_)).orElse { + try { + Option(Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF)) + } catch { + case e: Exception => + logError("Error loading default configuration file", e) + None } - case None => - try { - Some(Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF)) - } catch { - case e: Exception => { - logError("Error loading default configuration file", e) - None - } - } - }).foreach(is => + } + + isOpt.foreach { is => try { properties.load(is) } finally { is.close() } - ) + } propertyCategories = subProperties(properties, INSTANCE_REGEX) if (propertyCategories.contains(DEFAULT_PREFIX)) { From 94e810b73bb0eb2fcdea9a0ce9b4f47851ac8eeb Mon Sep 17 00:00:00 2001 From: Ryan Williams Date: Thu, 12 Feb 2015 20:47:42 +0000 Subject: [PATCH 3/7] throw if nonexistent Sink class is specified --- .../main/scala/org/apache/spark/metrics/MetricsSystem.scala | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala b/core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala index 83e8eb71260eb..345db36630fd5 100644 --- a/core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala +++ b/core/src/main/scala/org/apache/spark/metrics/MetricsSystem.scala @@ -191,7 +191,10 @@ private[spark] class MetricsSystem private ( sinks += sink.asInstanceOf[Sink] } } catch { - case e: Exception => logError("Sink class " + classPath + " cannot be instantialized", e) + case e: Exception => { + logError("Sink class " + classPath + " cannot be instantialized") + throw e + } } } } From ad24b0ee183d0c7f37d7973e5451bfcaaf3d5f5d Mon Sep 17 00:00:00 2001 From: Ryan Williams Date: Thu, 12 Feb 2015 20:46:26 +0000 Subject: [PATCH 4/7] add "metrics.properties" to .rat-excludes it's common to have this file laying around as the default config file for MetricsSystem, but RAT doesn't like its lack of an Apache header. --- .rat-excludes | 1 + 1 file changed, 1 insertion(+) diff --git a/.rat-excludes b/.rat-excludes index a788e8273d8a2..8c61e67a0c7d1 100644 --- a/.rat-excludes +++ b/.rat-excludes @@ -19,6 +19,7 @@ fairscheduler.xml.template spark-defaults.conf.template log4j.properties log4j.properties.template +metrics.properties metrics.properties.template slaves slaves.template From f4d7fab973e85110c544ce4c9e9d0654c4aa7a87 Mon Sep 17 00:00:00 2001 From: Ryan Williams Date: Fri, 13 Feb 2015 00:04:39 +0000 Subject: [PATCH 5/7] fix tests --- core/src/test/resources/test_metrics_system.properties | 2 -- .../scala/org/apache/spark/metrics/MetricsConfigSuite.scala | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/core/src/test/resources/test_metrics_system.properties b/core/src/test/resources/test_metrics_system.properties index 35d0bd3b8d0b8..4e8b8465696e5 100644 --- a/core/src/test/resources/test_metrics_system.properties +++ b/core/src/test/resources/test_metrics_system.properties @@ -18,7 +18,5 @@ *.sink.console.period = 10 *.sink.console.unit = seconds test.sink.console.class = org.apache.spark.metrics.sink.ConsoleSink -test.sink.dummy.class = org.apache.spark.metrics.sink.DummySink -test.source.dummy.class = org.apache.spark.metrics.source.DummySource test.sink.console.period = 20 test.sink.console.unit = minutes diff --git a/core/src/test/scala/org/apache/spark/metrics/MetricsConfigSuite.scala b/core/src/test/scala/org/apache/spark/metrics/MetricsConfigSuite.scala index 1a9ce8c607dcd..37e528435aa5d 100644 --- a/core/src/test/scala/org/apache/spark/metrics/MetricsConfigSuite.scala +++ b/core/src/test/scala/org/apache/spark/metrics/MetricsConfigSuite.scala @@ -27,7 +27,7 @@ class MetricsConfigSuite extends FunSuite with BeforeAndAfter { } test("MetricsConfig with default properties") { - val conf = new MetricsConfig(Option("dummy-file")) + val conf = new MetricsConfig(None) conf.initialize() assert(conf.properties.size() === 4) From 08ff99886cc6a45528a167968c8ab4ac3dcd4add Mon Sep 17 00:00:00 2001 From: Ryan Williams Date: Fri, 13 Feb 2015 23:04:47 +0000 Subject: [PATCH 6/7] rename METRICS_CONF: DEFAULT_METRICS_CONF_FILENAME --- .../main/scala/org/apache/spark/metrics/MetricsConfig.scala | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala index e8b017016d85e..55b156d3dd452 100644 --- a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala +++ b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala @@ -30,7 +30,7 @@ private[spark] class MetricsConfig(val configFile: Option[String]) extends Loggi val DEFAULT_PREFIX = "*" val INSTANCE_REGEX = "^(\\*|[a-zA-Z]+)\\.(.+)".r - val METRICS_CONF = "metrics.properties" + val DEFAULT_METRICS_CONF_FILENAME = "metrics.properties" val properties = new Properties() var propertyCategories: mutable.HashMap[String, Properties] = null @@ -49,7 +49,7 @@ private[spark] class MetricsConfig(val configFile: Option[String]) extends Loggi // If spark.metrics.conf is not set, try to get file in class path val isOpt: Option[InputStream] = configFile.map(new FileInputStream(_)).orElse { try { - Option(Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF)) + Option(Utils.getSparkClassLoader.getResourceAsStream(DEFAULT_METRICS_CONF_FILENAME)) } catch { case e: Exception => logError("Error loading default configuration file", e) From 5bccb142b85411b6c76f2faa0215d1f2dc85374e Mon Sep 17 00:00:00 2001 From: Ryan Williams Date: Fri, 13 Feb 2015 23:38:19 +0000 Subject: [PATCH 7/7] private-ize some MetricsConfig members --- .../scala/org/apache/spark/metrics/MetricsConfig.scala | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala index 55b156d3dd452..8edf493780687 100644 --- a/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala +++ b/core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala @@ -28,12 +28,12 @@ import org.apache.spark.util.Utils private[spark] class MetricsConfig(val configFile: Option[String]) extends Logging { - val DEFAULT_PREFIX = "*" - val INSTANCE_REGEX = "^(\\*|[a-zA-Z]+)\\.(.+)".r - val DEFAULT_METRICS_CONF_FILENAME = "metrics.properties" + private val DEFAULT_PREFIX = "*" + private val INSTANCE_REGEX = "^(\\*|[a-zA-Z]+)\\.(.+)".r + private val DEFAULT_METRICS_CONF_FILENAME = "metrics.properties" - val properties = new Properties() - var propertyCategories: mutable.HashMap[String, Properties] = null + private[metrics] val properties = new Properties() + private[metrics] var propertyCategories: mutable.HashMap[String, Properties] = null private def setDefaultProperties(prop: Properties) { prop.setProperty("*.sink.servlet.class", "org.apache.spark.metrics.sink.MetricsServlet")