Skip to content

Conversation

@LantaoJin
Copy link
Contributor

@LantaoJin LantaoJin commented May 20, 2017

…se the exception very confused

What changes were proposed in this pull request?

Spark Metrics System use a Properties File to load the configurations but doesn't trim the keys and values. It might cause the exception very confused if the property is a class name.
For example below, you must do not notice there is a space at the line end.

*.sink.ganglia.class=org.apache.spark.metrics.sink.GangliaSink

Unfortunately, the ClassNotFoundException throwing from Driver also doesn't tell me what happens and confuses me because I am sure the related jar is in the CLASSPATH.

17/05/20 12:47:04 ERROR SparkContext: Error initializing SparkContext.
java.lang.ClassNotFoundException: org.apache.spark.metrics.sink.GangliaSink
at scala.reflect.internal.util.AbstractFileClassLoader.findClass(AbstractFileClassLoader.scala:62)

As a reference, I check the code of Log4j, a classic Properties using library. It do the trim when load the properties. See org.apache.log4j.filter.PropertyFilter.java

private Hashtable parseProperties(String props) {
	Hashtable hashTable = new Hashtable();
	StringTokenizer pairs = new StringTokenizer(props, ",");
	while (pairs.hasMoreTokens()) {
		StringTokenizer entry = new StringTokenizer(pairs.nextToken(), "=");
		hashTable.put(entry.nextElement().toString().trim(), entry.nextElement().toString().trim());
	}
	return hashTable;
}

How was this patch tested?

Add unit tests

Also can test manually by setting metrics.properties file
(Replace "_" with " ")

*.sink.csv.class=_org.apache.spark.metrics.sink.CsvSink_

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@srowen
Copy link
Member

srowen commented May 20, 2017

Why do you think this relates to trimming whitespace?

@LantaoJin
Copy link
Contributor Author

@srowen It's not a real normal class not found case. And I do know what happened here. What I point out is a case that a whitespace at the end of the class name will cause ClassNotFound exception. This case is very confused to user. If it can be trimmed before reflection, that's much good I think.

@jerryshao
Copy link
Contributor

@LantaoJin I don't think you have to do trim for metrics conf coming from SparkConf:

  1. SparkConf already handles trim when reading from spark-defaults.conf file.
  2. If you deliberately add trailing WS from code like here, I think it's up to you to guarantee the correctness of such configuration.

For metrics conf reading from metrics property file, I think we could trim the trailing whitespace when reading from property file.

@srowen
Copy link
Member

srowen commented May 30, 2017

Let's close this

@jerryshao
Copy link
Contributor

@srowen , this issue existed when reading from metrics.properties conf file, I think we should fix this part. As for SparkConf part, I don't think it is necessary to fix.

@srowen srowen mentioned this pull request Jun 7, 2017
@asfgit asfgit closed this in b771fed Jun 8, 2017
zifeif2 pushed a commit to zifeif2/spark that referenced this pull request Nov 22, 2025
# What changes were proposed in this pull request?

This PR proposes to close stale PRs, mostly the same instances with apache#18017

Closes apache#11459
Closes apache#13833
Closes apache#13720
Closes apache#12506
Closes apache#12456
Closes apache#12252
Closes apache#17689
Closes apache#17791
Closes apache#18163
Closes apache#17640
Closes apache#17926
Closes apache#18163
Closes apache#12506
Closes apache#18044
Closes apache#14036
Closes apache#15831
Closes apache#14461
Closes apache#17638
Closes apache#18222

Added:
Closes apache#18045
Closes apache#18061
Closes apache#18010
Closes apache#18041
Closes apache#18124
Closes apache#18130
Closes apache#12217

Added:
Closes apache#16291
Closes apache#17480
Closes apache#14995

Added:
Closes apache#12835
Closes apache#17141

## How was this patch tested?

N/A

Author: hyukjinkwon <gurwls223@gmail.com>

Closes apache#18223 from HyukjinKwon/close-stale-prs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants