Update SQLConf.scala

[baishuo]

use concurrent.ConcurrentHashMap instead of util.Collections.synchronizedMap

[AmplabJenkins]

Can one of the admins verify this patch?

[aarondav]

Jenkins, test this please.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[aarondav]

Out of curiosity, what motivates this change? I thought typically ConcurrentHashMap is used for more heavyweight concurrent data structures, while synchronizedMap() is used when concurrency is rare.

Either way, our usage is not threadsafe, since we do things like

if (map.contains(x)) map.get(x) else defaultValue

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16277/

[baishuo]

I add some synchronized please see if it is thread safe，and Jenkins should test this once more

[aarondav]

This is indeed threadsafe, but perhaps overeager. I think we should aim for get()s to be relatively fast, and I think we can avoid extra synchronization there.

[aarondav]

Perhaps we can remove this synchronized, I don't think we care about the consistency guarantees of inserting multiple properties at once :)

[cloud-fan]

With the new synchronizeds you added in usage, I don't think we need ConcurrentHashMap any more. Maybe just a simple HashMap is enough.

[aarondav]

Similarly, here, just

Option(settings.get(key))

[baishuo]

thanks @aarondav ，had modified according to your comment，please help me to check if it is proper

[rxin]

Can you undo the indent spacing change?

[chenghao-intel]

Probably adding the checking logic is better.

[cloud-fan]

Agree. How about Option(settings.get(key)).orElse(throw new NoSuchElementException(key))?

[chenghao-intel]

And I also saw the code:

def toDebugString: String = {
settings.synchronized {
settings.asScala.toArray.sorted.map{ case (k, v) => s"$k=$v" }.mkString("\n")
}
}

Should we remove the synchronized block also? since we use the ConcurrentHashMap instead.

[rxin]

We should not be using ConcurrentHashMap because this will be a very low contention code path. For low contention code path, ConcurrentHashMap is a very poor choice (as a matter of fact it'll likely be much slower than synchronized, and use a lot more memory)

[baishuo]

back to Collections.synchronizedMap

[aarondav]

@rxin I think the performance distinction is extremely minor in this case, as there is only one ConcurrentHashMap. ConcurrentHashMap's API tends to be nicer to use, though, as people may not realize that iteration over a SynchronizedMap is not threadsafe, like in the current implementation of SQLConf.

As @baishuo mentioned, if we use synchronizedMap we'll have to add settings.synchronized {} in a few places now.

[baishuo]

Hi,@rxin ，had remove indent spacing on
def set(props: Properties): Unit = {
props.asScala.foreach { case (k, v) => this.settings.put(k, v) }
}
please help me to check if it is proper

[baishuo]

hi @rxin, how to modify is proper?
use java.util.Collections.synchronizedMap and
use settings.sycnronize {
...............
}
to ensure the thread safe?

[concretevitamin]

Yeah, what is motivating this change? When this class got introduced, @rxin commented that java.util.ConcurrentHashMap had bad memory footprint and suggested the current approach instead.

[concretevitamin]

Sorry, I didn't realize Reynold had already commented on this thread. The current changes with Option look good.

[concretevitamin]

Jenkins, ok to test.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16302/

[aarondav]

@rxin wins because he says that SQLConf will become a Thread-local variable. This looks good, the only thing to change for thread-safety is to add a synchronized for getAll().

[rxin]

Note that the latest code no longer compiles ....

[rxin]

this line should be getOrElse

[cloud-fan]

This follows code style from BlockManager:

 def getLocalFromDisk(blockId: BlockId, serializer: Serializer): Option[Iterator[Any]] = {
diskStore.getValues(blockId, serializer).orElse(
sys.error("Block " + blockId + " not found on disk, though it should be"))
}

Anyway throw expression return Nothing, which can work with both getOrElse and orElse

[aarondav]

orElse literally does not compile here, as it returns Option(Nothing).

[cloud-fan]

Ah, I see. get(key: String) need to return a String which I missed. My bad :P

[baishuo]

ooh，sorry about the compile error，had change orElse to getOrElse. thank you @rxin

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished.

[AmplabJenkins]

Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16335/

[rxin]

Jenkins, retest this please.

[AmplabJenkins]

Merged build triggered.

[AmplabJenkins]

Merged build started.

[AmplabJenkins]

Merged build finished. All automated tests passed.

[AmplabJenkins]

All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16338/

[rxin]

Thanks. I'm merging this in master & branch-1.0.