SPARK-5425: Use synchronised methods in system properties to create SparkConf

[jacek-lewandowski]

SPARK-5425: Fixed usages of system properties

This patch fixes few problems caused by the fact that the Scala wrapper over system properties is not thread-safe and is basically invalid because it doesn't take into account the default values which could have been set in the properties object. The problem is fixed by modifying Utils.getSystemProperties method so that it uses stringPropertyNames method of the Properties class, which is thread-safe (internally it creates a defensive copy in a synchronized method) and returns keys of the properties which were set explicitly and which are defined as defaults.
The other related problem, which is fixed here. was in ResetSystemProperties mix-in. It created a copy of the system properties in the wrong way.

This patch also introduces a test case for thread-safeness of SparkConf creation.

Refer to the discussion in #4220 for more details.

[SparkQA]

QA tests have started for PR 4220 at commit 685780e.

• This patch merges cleanly.

[srowen]

It does look safer, since stringPropertyNames will create a copy of the keys.

[jacek-lewandowski]

@srowen exactly - that was the idea :)

[SparkQA]

QA tests have finished for PR 4220 at commit 685780e.

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

[AmplabJenkins]

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

[jacek-lewandowski]

What is going on with these tests??? I've created three PRs - for 1.1, 1.2 and 1.3 and all of them failed in a very strange way.

[vanzin]

ultra nit: sort imports

[vanzin]

LGTM. I was worried about System.getProperty()'s thread-safety, but I assume it's ultimately synchronized since the underlying store is a Properties object.

[squito]

retest this please

hopefully those test failures were random, lets see. btw, I think that if you want the exact same patch applied to multiple branches, the standard practice is to just open one PR and make a comment that it should be backported to other branches. Its easy for committers to apply to multiple branches. Makes it easier to track the PRs. though this done mean we assume that if the test pass on master, they'll pass on other branches.

(somebody correct me if I'm mistaken here ...)

[squito]

retest this please

[SparkQA]

QA tests have started for PR 4220 at commit 685780e.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 4220 at commit 685780e.

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

[AmplabJenkins]

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

[squito]

I kinda see what is going on with the tests now. A test case in SparkSubmitSuite adds a non-existent jar to the system properties. That property never gets cleared, so later jobs which should submit successfully, are still trying to load that jar.

I don't understand how your change is effecting this, though, or how this ever worked before. Maybe this change is exposing some lurking error -- something which just "happened" to work before. It seems like having multiple apps futzing w/ the system properties at the same time is bound to create problems.

[vanzin]

Kinda diverging into off-topic territory, but...

It seems like having multiple apps futzing w/ the system properties at the same time is bound to create problems.

Yeah, this is one of those things that keep ringing in my head all the time. I think we should have a plan to have some Spark-specific entry point (e.g. main(args: Array[String], conf: SparkConf)) that is used in lieu of the usual main if it's found. That could prevent having SparkSubmit and tests mess with system properties at all.

But that's for another PR, this one looks good as it is (perhaps after fixing the offending test).

[jacek-lewandowski]

I found that there is a small difference in the outcome of my code in comparison to the original code. I don't know how it affect this test suite yet, but: the default Java wrapper over the system properties, which was used originally uses entrySet method to get the properties. It means that when there are some defaults defined, there are not used. On the other hand, stringPropertyNames method returns a set of property keys - both defined explicitly and by providing default values. The same for System.getProperty method. The later behaviour seems more appropriate here.

[jacek-lewandowski]

It looks like there is ResetSystemProperties trait which works completely wrong. Its aim is to save system properties before the test and then reset them after the test. However, the way the copy was created and then restored was invalid.

The copy was created in this way:

oldProperties = new Properties(System.getProperties)

which did not initialize properties as they were in the original system properties but rather set them as defaults in new properties object, which in turn made JPropertiesWrapper not consider them at all (actually JPropertiesWrapper would return empty iterator then).

The way I'm gonna fix ResetSystemProperties class is to use SerializationUtils.clone to save the system properties.

[SparkQA]

QA tests have started for PR 4220 at commit d821b15.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 4220 at commit d821b15.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

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

[squito]

thanks for tracking this down. can you please put a comment in here explaining why you need a clone? it is really subtle, I can easily see this getting reverted down the road if somebody doesn't know why its there.

[SparkQA]

QA tests have started for PR 4220 at commit 74b4489.

• This patch merges cleanly.

[squito]

lgtm

[SparkQA]

QA tests have finished for PR 4220 at commit 74b4489.

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

[AmplabJenkins]

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

[jacek-lewandowski]

@JoshRosen can you take a look and maybe merge it?

[vanzin]

Could we get some closure on this? The broken ResetSystemProperties causes core/ tests to fill up my disk currently (and run a lot more slowly than needed), because of bad interaction with other tests.

[srowen]

@jacek-lewandowski a Properties is just a Hashtable so you can make a new Properties and addAll. No need to even clone.

[jacek-lewandowski]

@srowen - unfortunately they are something more - they inherit from the HashTable but they makes a hierarchy by referencing the parent Properties which are the defaults. As the defaults is also Properties, it has its own parent and so on.

[vanzin]

@jacek-lewandowski I think Sean meant that you can do new Properties().putAll(oldProperties) instead of cloning.

[jacek-lewandowski]

Look at this simple example:

val parent = new Properties()
parent.setProperty("test1", "A")

val child = new Properties(parent)
child.put("test2", "B")

val copy = new Properties()
copy.putAll(child)

child.getProperty("test1")
child.getProperty("test2")

copy.getProperty("test1")
copy.getProperty("test2")

which will result in:

scala> res3: String = A
scala> res4: String = B
scala> res5: String = null
scala> res6: String = B

In other words: new Properties(oldProperties) initialises a new properties by setting oldProperties as a parent (defaults). On the other hand new Properties().putAll(oldProperties) copies only those properties which were explicitly set and cuts the whole hierarchy with defaults. Only cloning gives you the same object.

[vanzin]

I see, makes sense, thanks for the details.

[srowen]

Ah OK, interesting. On another note, does the clone make a deep copy? that's necessary I think?
Anyway, worst case, you can iterate over stringPropertyNames and put into a new Properties.

[jacek-lewandowski]

It serializes the object and then deserializes so I suppose this is a deep copy.

For the stringPropertyNames - you can, but this will not be a 1:1 copy:

val parent = new Properties()
parent.setProperty("test1", "A")

val child = new Properties(parent)
child.put("test1", "C")
child.put("test2", "B")

child.getProperty("test1")
child.remove("test1")
child.getProperty("test1")

will give you

scala> res17: Object = C
scala> res18: String = A

When you copy in the way you suggested, there will be null after removal.

[andrewor14]

@JoshRosen who investigated this a bunch for tests

[JoshRosen]

I did a little digging and it looks like this ConcurrentModificationException error was fixed previously (8f1098a) but that the fix may have been lost as part of the migration to SparkConf. It looks like #3788 addressed a similar issue for another usage of System.getProperties. For uniformity's sake, what do you think about using Utils.getSystemProperties here instead of stringProperties?

Thanks for adding the regression test.

The bug in ResetSystemProperties was a silly mistake on my part, which was introduced in https://issues.apache.org/jira/browse/SPARK-1010; thanks for fixing that.

If / when you update this PR, mind adding a one- or two-sentence description that will become the commit message?

Once we've done that and decided whether to use Utils.getSystemProperties, let's merge this into master and all of the maintenance branches to fix the test issues.

[jacek-lewandowski]

Utils.getSystemProperties is fine but not as a replacement of stringProperties. There were actually two problems - one was related to ConcurrentModificationException and the other one was related to abandoning completely the defaults. Unfortunately Utils.getSystemProperties is not enough because it doesn't make Scala wrapper over Java properties (which is used when you just iterate over them) to consider default values defined for the properties (as I said - they make a hierarchy). Since you still need to use stringPropertyNames which creates a defensive copy anyway, cloning properties before you want to use them seems to be redundant. Maybe a more clear solution would be to modify Utils.getSystemProperties to use stringPropertyNames to make a copy and return just a simple Map instead?

[JoshRosen]

Maybe a more clear solution would be to modify Utils.getSystemProperties to use stringPropertyNames to make a copy and return just a simple Map instead?

This sounds reasonable to me; intuitively, it seems like we'd like the map returned from Utils.getSystemProperties to reflect the current values of all properties regardless of whether they're explicitly defined in the current properties object or inherited from something further up in the override hierarchy.

This is pretty subtle, though, so it would be good to add a comment and a mention of the JIRA number so that future readers can understand these subtleties and not unknowingly break things.

[jacek-lewandowski]

Agreed, I'll make the changes and add the clarifying comments.

[SparkQA]

QA tests have started for PR 4220 at commit 6c48a1f.

• This patch merges cleanly.

[SparkQA]

QA tests have finished for PR 4220 at commit 6c48a1f.

• This patch passes unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[AmplabJenkins]

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

[jacek-lewandowski]

@JoshRosen is it ok to go now?

[JoshRosen]

@jacek-lewandowski This looks good to me, so I'll merge it in a few minutes. It looks like your PRs against the other branches have also been updated, so I'll pull those in, too. Thanks!

[JoshRosen]

I've merged this PR into branch-1.1 (1.1.2), so feel free to close it (GitHub won't auto-close PRs that are opened against maintenance branches).