[SPARK-6048] SparkConf should not translate deprecated configs on set by andrewor14 · Pull Request #4799 · apache/spark

andrewor14 · 2015-02-27T01:00:03Z

There are multiple issues with translating on set outlined in the JIRA.

This PR reverts the translation logic added to SparkConf. In the future, after the 1.3.0 release we will figure out a way to reorganize the internal structure more elegantly. For now, let's preserve the existing semantics of SparkConf since it's a public interface. Unfortunately this means duplicating some code for now, but this is all internal and we can always clean it up later.

SparkQA · 2015-02-27T01:03:01Z

Test build #28029 has started for PR 4799 at commit 94b4dfa.

This patch merges cleanly.

andrewor14 · 2015-02-27T01:03:30Z

cc @vanzin who originally wrote this code

SparkQA · 2015-02-27T01:59:57Z

Test build #28029 has finished for PR 4799 at commit 94b4dfa.

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

AmplabJenkins · 2015-02-27T02:00:01Z

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

pwendell · 2015-02-27T02:35:45Z

@vanzin does this look okay to you? I commented on the JIRA, but my main goal is to find a surgical patch here that can unblock the release, so just rewinding the change seems the safest.

SparkQA · 2015-02-27T02:37:59Z

Test build #28037 has started for PR 4799 at commit fef6c9c.

This patch merges cleanly.

SparkQA · 2015-02-27T03:59:08Z

Test build #28037 has finished for PR 4799 at commit fef6c9c.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-27T03:59:12Z

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

SparkQA · 2015-02-28T02:08:01Z

Test build #28104 has started for PR 4799 at commit c26a9e3.

This patch merges cleanly.

vanzin · 2015-02-28T02:38:31Z

Given the discussion on the bug it looks ok, but it's missing the documentation that explains the behavior.

SparkQA · 2015-02-28T03:28:13Z

Test build #28104 has finished for PR 4799 at commit c26a9e3.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-02-28T03:28:17Z

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

pwendell · 2015-03-01T02:20:26Z

I agree @andrewor14 can you add documentation about deprecated configs?

I would extend what's there now:

Properties set directly on the SparkConf take highest precedence, then flags passed to spark-submit or spark-shell, then options in the spark-defaults.conf file. A few configuration keys have been renamed
since earlier versions of Spark; in such cases, the older key names are still
accepted, but take lower precedence than any instance of the newer key.

SparkQA · 2015-03-01T07:12:42Z

Test build #28143 has started for PR 4799 at commit 10e77b5.

This patch merges cleanly.

SparkQA · 2015-03-01T08:30:17Z

Test build #28143 has finished for PR 4799 at commit 10e77b5.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-01T08:30:21Z

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

pwendell · 2015-03-01T23:53:40Z

This will log it on the executors, which seems weird since all other configuration warnings are logged at the driver or submission client. Can we just log this warning on the driver side? Or just don't log a deprecation warning always?

Is your proposal to not log this warning, or log it on the driver side?

Log it on the driver side during validation, or at some earlier point. I just think having it here isn't super useufl.

SparkQA · 2015-03-02T18:47:44Z

Test build #28178 has started for PR 4799 at commit 11c525b.

This patch merges cleanly.

SparkQA · 2015-03-02T20:04:18Z

Test build #28178 has finished for PR 4799 at commit 11c525b.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

AmplabJenkins · 2015-03-02T20:04:22Z

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

vanzin · 2015-03-02T22:03:23Z

Since you're doing this only once during the app's lifetime (this method is only called from SparkContext AFAICT), you could simplify the code that tracks whether warns have been printed in DeprecatedConfig. But ok to not do it if you want to limit the scope of the change.

yeah let's revamp this whole thing after the release

vanzin · 2015-03-02T22:05:19Z

LGTM, left just minor comments.

pwendell · 2015-03-03T00:36:28Z

Okay - thanks everyone for helping with this. I'll pull it in.

There are multiple issues with translating on set outlined in the JIRA. This PR reverts the translation logic added to `SparkConf`. In the future, after the 1.3.0 release we will figure out a way to reorganize the internal structure more elegantly. For now, let's preserve the existing semantics of `SparkConf` since it's a public interface. Unfortunately this means duplicating some code for now, but this is all internal and we can always clean it up later. Author: Andrew Or <andrew@databricks.com> Closes#4799 from andrewor14/conf-set-translate and squashes the following commits: 11c525b [Andrew Or] Move warning to driver 10e77b5 [Andrew Or] Add documentation for deprecation precedence a369cb1 [Andrew Or] Merge branch 'master' of github.com:apache/spark into conf-set-translate c26a9e3 [Andrew Or] Revert all translate logic in SparkConf fef6c9c [Andrew Or] Restore deprecation logic for spark.executor.userClassPathFirst 94b4dfa [Andrew Or] Translate on get, not set (cherry picked from commit 258d154) Signed-off-by: Patrick Wendell <patrick@databricks.com>

Translate on get, not set
94b4dfa

andrewor14 mentioned this pull request Feb 27, 2015
[SPARK-6046] Privatize SparkConf.translateConfKey #4797
Closed

Restore deprecation logic for spark.executor.userClassPathFirst
fef6c9c

Revert all translate logic in SparkConf
c26a9e3

Andrew Or added 2 commits February 28, 2015 23:04

Merge branch 'master' of github.com:apache/spark into conf-set-translate
a369cb1

Add documentation for deprecation precedence
10e77b5

pwendell reviewed Mar 1, 2015
View reviewed changes

Move warning to driver
11c525b

vanzin reviewed Mar 2, 2015
View reviewed changes

asfgit closed this in 258d154Mar 3, 2015

andrewor14 deleted the conf-set-translate branch March 3, 2015 00:49

zsxwing mentioned this pull request Mar 24, 2015
[SPARK-5124][Core] A standard RPC interface and an Akka implementation #4588
Closed

Uh oh!

Conversation

andrewor14 commented Feb 27, 2015

Uh oh!

SparkQA commented Feb 27, 2015

Uh oh!

andrewor14 commented Feb 27, 2015

Uh oh!

SparkQA commented Feb 27, 2015

Uh oh!

AmplabJenkins commented Feb 27, 2015

Uh oh!

pwendell commented Feb 27, 2015

Uh oh!

SparkQA commented Feb 27, 2015

Uh oh!

SparkQA commented Feb 27, 2015

Uh oh!

AmplabJenkins commented Feb 27, 2015

Uh oh!

SparkQA commented Feb 28, 2015

Uh oh!

vanzin commented Feb 28, 2015

Uh oh!

SparkQA commented Feb 28, 2015

Uh oh!

AmplabJenkins commented Feb 28, 2015

Uh oh!

pwendell commented Mar 1, 2015

Uh oh!

SparkQA commented Mar 1, 2015

Uh oh!

SparkQA commented Mar 1, 2015

Uh oh!

AmplabJenkins commented Mar 1, 2015

Uh oh!

pwendellMar 1, 2015

Choose a reason for hiding this comment

Uh oh!

andrewor14Mar 2, 2015

Choose a reason for hiding this comment

Uh oh!

pwendellMar 2, 2015

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Mar 2, 2015

Uh oh!

SparkQA commented Mar 2, 2015

Uh oh!

AmplabJenkins commented Mar 2, 2015

Uh oh!

vanzinMar 2, 2015

Choose a reason for hiding this comment

Uh oh!

andrewor14Mar 2, 2015

Choose a reason for hiding this comment

Uh oh!

vanzin commented Mar 2, 2015

Uh oh!

pwendell commented Mar 3, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants