[SPARK-5778] throw if nonexistent metrics config file provided #4571

ryan-williams · 2015-02-12T19:27:05Z

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.

SparkQA · 2015-02-12T19:32:32Z

Test build #27368 has started for PR 4571 at commit 105288e.

This patch merges cleanly.

SparkQA · 2015-02-12T19:54:32Z

Test build #27368 has finished for PR 4571 at commit 105288e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-12T19:54:36Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27368/
Test FAILed.

rxin · 2015-02-12T20:35:23Z

core/src/main/scala/org/apache/spark/metrics/MetricsConfig.scala

-      is = configFile match {
-        case Some(f) => new FileInputStream(f)
-        case None => Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF)
+    (configFile match {


I've been thinking about style quite a bit recently as I'm working on a style guide. In this case, you'd want to break the chain down with intermediate variables. Otherwise the cognitive complexity of the chaining/nesting makes it hard to understand.

I wrote 3 versions of them, with each version gradually becoming more Java-like. I think any of them would be fine here.

version 1

// FileInputStream constructor throws FileNotFoundException if file doesn't exist 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 } } isOpt.foreach { is => try { properties.load(is) } finally { is.close() } }

version 2

// FileInputStream constructor throws FileNotFoundException if file doesn't exist var isOpt: Option[InputStream] = configFile.map { f => logInfo("Loading MetricsConfig file: $f") new FileInputStream(f) } if (!isOpt.isDefined) { try { is = Option(Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF)) } catch { case e: Exception => logError("Error loading default configuration file", e) } } isOpt.foreach { is => try { properties.load(is) } finally { is.close() } }

version 3

// FileInputStream constructor throws FileNotFoundException if file doesn't exist var is: InputStream = configFile.map(new FileInputStream(_)).orNull if (is == null) { try { is = Option(Utils.getSparkClassLoader.getResourceAsStream(METRICS_CONF)) } catch { case e: Exception => logError("Error loading default configuration file", e) } } if (is != null) { try { properties.load(is) } finally { is.close() } }

cool, thanks, went with the first one.

incidentally I had a Some() where I should have had an Option(), which your candidates all fix, that I think was breaking the tests

SparkQA · 2015-02-12T20:57:33Z

Test build #27376 has started for PR 4571 at commit b98e325.

This patch merges cleanly.

ryan-williams · 2015-02-12T21:00:32Z

I threw in a couple of other trivial things:

throw if a bad Sink class is specified
exclude conf/metrics.properties, if present, from RAT checks

lmk if you want me to make those separate JIRAs/PRs

SparkQA · 2015-02-12T21:02:27Z

Test build #27378 has started for PR 4571 at commit 45f1d3e.

This patch merges cleanly.

SparkQA · 2015-02-12T21:54:57Z

Test build #27376 has finished for PR 4571 at commit b98e325.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-12T21:55:01Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27376/
Test FAILed.

SparkQA · 2015-02-12T21:56:38Z

Test build #27378 has finished for PR 4571 at commit 45f1d3e.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- logError("Sink class " + classPath + " cannot be instantialized")

AmplabJenkins · 2015-02-12T21:56:42Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27378/
Test FAILed.

SparkQA · 2015-02-13T00:07:33Z

Test build #27399 has started for PR 4571 at commit 1b74f69.

This patch merges cleanly.

SparkQA · 2015-02-13T01:18:22Z

Test build #27399 has finished for PR 4571 at commit 1b74f69.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- logError("Sink class " + classPath + " cannot be instantialized")

AmplabJenkins · 2015-02-13T01:18:26Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27399/
Test PASSed.

rxin · 2015-02-13T01:20:38Z

This looks good. @pwendell I'm not sure which branch you want this in, so I will let you do the merge.

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.

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.

ryan-williams · 2015-02-13T23:39:34Z

I just rebased and tacked a couple of trivial commits to MetricsConfig onto the end of this, jfyi

rxin · 2015-02-13T23:40:37Z

Jenkins, retest this please.

SparkQA · 2015-02-13T23:42:33Z

Test build #27464 has started for PR 4571 at commit 5bccb14.

This patch merges cleanly.

SparkQA · 2015-02-14T00:40:46Z

Test build #27464 has finished for PR 4571 at commit 5bccb14.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- logError("Sink class " + classPath + " cannot be instantialized")
- case class Params(

AmplabJenkins · 2015-02-14T00:40:50Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27464/
Test FAILed.

ryan-williams · 2015-02-14T01:15:55Z

looks like similar sorts of probably-spurious to those that I saw on #4573; thoughts, @rxin? re-test?

rxin · 2015-02-14T02:03:46Z

Jenkins, retest this please

SparkQA · 2015-02-14T02:07:26Z

Test build #27468 has started for PR 4571 at commit 5bccb14.

This patch merges cleanly.

SparkQA · 2015-02-14T02:56:21Z

Test build #27468 has finished for PR 4571 at commit 5bccb14.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- logError("Sink class " + classPath + " cannot be instantialized")

AmplabJenkins · 2015-02-14T02:56:24Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27468/
Test FAILed.

ryan-williams · 2015-02-14T22:54:28Z

seems like another spurious failure to me. That one passed in the previous build

pwendell · 2015-02-15T17:32:38Z

Jenkins, retest this please. (sorry @ryan-williams we're trying to get tests in better shape).

SparkQA · 2015-02-15T17:37:33Z

Test build #27529 has started for PR 4571 at commit 5bccb14.

This patch merges cleanly.

pwendell · 2015-02-15T17:38:53Z

Overall, looks good to me. I do like the idea of failing here, since otherwise it can be very confusing to people why their settings are not being picked up. To avoid surprise here I think it's best to avoid backporting this patch (once merged) into older versions and just enforce it in new versions.

One other issue, it's not super well documented in the existing code base what type of error propagation we expect when initializing a spark environment on the executors. It would be good to verify that this gets thrown up the chain in a way that gets presented nicely to the user. I wouldn't block the patch on that (it's not newly introduced here), but having nicer internal documentation about the expectations that callers should have if SparkEnv's fail to initialize would be nice.

SparkQA · 2015-02-15T18:46:54Z

Test build #27529 has finished for PR 4571 at commit 5bccb14.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
- logError("Sink class " + classPath + " cannot be instantialized")

AmplabJenkins · 2015-02-15T18:46:57Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/27529/
Test PASSed.

hammer · 2015-02-17T17:55:39Z

@pwendell @rxin looks ready to merge?

pwendell · 2015-02-17T18:57:29Z

Pulling it in now, thanks ryan!

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. Author: Ryan Williams <[email protected]> Closes #4571 from ryan-williams/metrics and squashes the following commits: 5bccb14 [Ryan Williams] private-ize some MetricsConfig members 08ff998 [Ryan Williams] rename METRICS_CONF: DEFAULT_METRICS_CONF_FILENAME f4d7fab [Ryan Williams] fix tests ad24b0e [Ryan Williams] add "metrics.properties" to .rat-excludes 94e810b [Ryan Williams] throw if nonexistent Sink class is specified 31d2c30 [Ryan Williams] metrics code review feedback 56287db [Ryan Williams] throw if nonexistent metrics config file provided (cherry picked from commit d8f69cf) Signed-off-by: Patrick Wendell <[email protected]>

rxin reviewed Feb 12, 2015
View reviewed changes

ryan-williams added 7 commits February 13, 2015 22:27

metrics code review feedback

31d2c30

throw if nonexistent Sink class is specified

94e810b

add "metrics.properties" to .rat-excludes

ad24b0e

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.

fix tests

f4d7fab

rename METRICS_CONF: DEFAULT_METRICS_CONF_FILENAME

08ff998

private-ize some MetricsConfig members

5bccb14

ryan-williams force-pushed the metrics branch from 1b74f69 to 5bccb14 Compare February 13, 2015 23:38

asfgit closed this in d8f69cf Feb 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-5778] throw if nonexistent metrics config file provided #4571

[SPARK-5778] throw if nonexistent metrics config file provided #4571

ryan-williams commented Feb 12, 2015

SparkQA commented Feb 12, 2015

SparkQA commented Feb 12, 2015

AmplabJenkins commented Feb 12, 2015

rxin Feb 12, 2015

ryan-williams Feb 12, 2015

SparkQA commented Feb 12, 2015

ryan-williams commented Feb 12, 2015

SparkQA commented Feb 12, 2015

SparkQA commented Feb 12, 2015

AmplabJenkins commented Feb 12, 2015

SparkQA commented Feb 12, 2015

AmplabJenkins commented Feb 12, 2015

SparkQA commented Feb 13, 2015

SparkQA commented Feb 13, 2015

AmplabJenkins commented Feb 13, 2015

rxin commented Feb 13, 2015

ryan-williams commented Feb 13, 2015

rxin commented Feb 13, 2015

SparkQA commented Feb 13, 2015

SparkQA commented Feb 14, 2015

AmplabJenkins commented Feb 14, 2015

ryan-williams commented Feb 14, 2015

rxin commented Feb 14, 2015

SparkQA commented Feb 14, 2015

SparkQA commented Feb 14, 2015

AmplabJenkins commented Feb 14, 2015

ryan-williams commented Feb 14, 2015

pwendell commented Feb 15, 2015

SparkQA commented Feb 15, 2015

pwendell commented Feb 15, 2015

SparkQA commented Feb 15, 2015

AmplabJenkins commented Feb 15, 2015

hammer commented Feb 17, 2015

pwendell commented Feb 17, 2015

[SPARK-5778] throw if nonexistent metrics config file provided #4571

[SPARK-5778] throw if nonexistent metrics config file provided #4571

Conversation

ryan-williams commented Feb 12, 2015

SparkQA commented Feb 12, 2015

SparkQA commented Feb 12, 2015

AmplabJenkins commented Feb 12, 2015

rxin Feb 12, 2015

Choose a reason for hiding this comment

ryan-williams Feb 12, 2015

Choose a reason for hiding this comment

SparkQA commented Feb 12, 2015

ryan-williams commented Feb 12, 2015

SparkQA commented Feb 12, 2015

SparkQA commented Feb 12, 2015

AmplabJenkins commented Feb 12, 2015

SparkQA commented Feb 12, 2015

AmplabJenkins commented Feb 12, 2015

SparkQA commented Feb 13, 2015

SparkQA commented Feb 13, 2015

AmplabJenkins commented Feb 13, 2015

rxin commented Feb 13, 2015

ryan-williams commented Feb 13, 2015

rxin commented Feb 13, 2015

SparkQA commented Feb 13, 2015

SparkQA commented Feb 14, 2015

AmplabJenkins commented Feb 14, 2015

ryan-williams commented Feb 14, 2015

rxin commented Feb 14, 2015

SparkQA commented Feb 14, 2015

SparkQA commented Feb 14, 2015

AmplabJenkins commented Feb 14, 2015

ryan-williams commented Feb 14, 2015

pwendell commented Feb 15, 2015

SparkQA commented Feb 15, 2015

pwendell commented Feb 15, 2015

SparkQA commented Feb 15, 2015

AmplabJenkins commented Feb 15, 2015

hammer commented Feb 17, 2015

pwendell commented Feb 17, 2015