You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by baishuo <gi...@git.apache.org> on 2014/07/01 05:29:59 UTC
[GitHub] spark pull request: Update SQLConf.scala
GitHub user baishuo opened a pull request:
https://github.com/apache/spark/pull/1272
Update SQLConf.scala
use concurrent.ConcurrentHashMap instead of util.Collections.synchronizedMap
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/baishuo/spark master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/1272.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1272
----
commit 0740f28b04a43ac739d6f45a7ffc6fa23fe7b96c
Author: baishuo(白硕) <vc...@hotmail.com>
Date: 2014-07-01T03:29:12Z
Update SQLConf.scala
use concurrent.ConcurrentHashMap instead of util.Collections.synchronizedMap
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47614369
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
```scala
if (map.contains(x)) map.get(x) else defaultValue
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48005588
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48005595
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48009712
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14546036
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +64,17 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
- throw new NoSuchElementException(key)
- }
- settings.get(key)
+ Option(settings.get(key)).orElse(throw new NoSuchElementException(key))
--- End diff --
orElse literally does not compile here, as it returns Option(Nothing).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47866583
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14541981
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +64,17 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
- throw new NoSuchElementException(key)
- }
- settings.get(key)
+ Option(settings.get(key)).orElse(throw new NoSuchElementException(key))
--- End diff --
this line should be getOrElse
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14391912
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -50,11 +50,10 @@ trait SQLConf {
/** ********************** SQLConf functionality methods ************ */
@transient
- private val settings = java.util.Collections.synchronizedMap(
- new java.util.HashMap[String, String]())
+ private val settings = new java.util.concurrent.ConcurrentHashMap[String, String]()
def set(props: Properties): Unit = {
- props.asScala.foreach { case (k, v) => this.settings.put(k, v) }
+ props.asScala.foreach { case (k, v) => this.settings.put(k, v) }
--- End diff --
can you reset the change
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47866584
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16302/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47613493
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/1272
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14392261
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -50,8 +50,7 @@ trait SQLConf {
/** ********************** SQLConf functionality methods ************ */
@transient
- private val settings = java.util.Collections.synchronizedMap(
- new java.util.HashMap[String, String]())
+ private val settings = new java.util.concurrent.ConcurrentHashMap[String, String]()
--- End diff --
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)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47970749
@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().
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47866381
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47625269
thanks @aarondav ，had modified according to your comment，please help me to check if it is proper
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14545788
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +64,17 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
- throw new NoSuchElementException(key)
- }
- settings.get(key)
+ Option(settings.get(key)).orElse(throw new NoSuchElementException(key))
--- End diff --
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`
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47866266
Jenkins, ok to test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47619393
Merged build finished.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47865791
modify according @cloud-fan ‘s comment
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47624200
With the new `synchronized`s you added in usage, I don't think we need `ConcurrentHashMap` any more. Maybe just a simple `HashMap` is enough.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47626937
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47626045
Can you undo the indent spacing change?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47627343
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
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14392109
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +63,17 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
--- End diff --
Probably adding the checking logic is better.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47614338
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47621874
I add some synchronized please see if it is thread safe，and Jenkins should test this once more
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14392768
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -50,8 +50,7 @@ trait SQLConf {
/** ********************** SQLConf functionality methods ************ */
@transient
- private val settings = java.util.Collections.synchronizedMap(
- new java.util.HashMap[String, String]())
+ private val settings = new java.util.concurrent.ConcurrentHashMap[String, String]()
--- End diff --
undo to Collections.synchronizedMap
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14546666
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +64,17 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
- throw new NoSuchElementException(key)
- }
- settings.get(key)
+ Option(settings.get(key)).orElse(throw new NoSuchElementException(key))
--- End diff --
Ah, I see. `get(key: String)` need to return a `String` which I missed. My bad :P
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47991057
Note that the latest code no longer compiles ....
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47614301
Jenkins, test this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48010506
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48005584
ooh，sorry about the compile error，had change orElse to getOrElse. thank you @rxin
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47717333
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48009714
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16335/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14391299
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -50,11 +50,12 @@ trait SQLConf {
/** ********************** SQLConf functionality methods ************ */
@transient
- private val settings = java.util.Collections.synchronizedMap(
- new java.util.HashMap[String, String]())
+ private val settings = new java.util.concurrent.ConcurrentHashMap[String, String]()
def set(props: Properties): Unit = {
- props.asScala.foreach { case (k, v) => this.settings.put(k, v) }
+ settings.synchronized {
--- End diff --
Perhaps we can remove this synchronized, I don't think we care about the consistency guarantees of inserting multiple properties at once :)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47619394
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16277/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48010392
Jenkins, retest this please.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48015483
All automated tests passed.
Refer to this link for build results: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16338/
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48015480
Merged build finished. All automated tests passed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47614328
Merged build triggered.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47634643
hi @rxin, how to modify is proper?
use settings.sycnronize {
...............
}
to ensure the thread safe?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14438619
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +63,17 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
--- End diff --
Agree. How about `Option(settings.get(key)).orElse(throw new NoSuchElementException(key))`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47866389
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14407762
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -50,8 +50,7 @@ trait SQLConf {
/** ********************** SQLConf functionality methods ************ */
@transient
- private val settings = java.util.Collections.synchronizedMap(
- new java.util.HashMap[String, String]())
+ private val settings = new java.util.concurrent.ConcurrentHashMap[String, String]()
--- End diff --
@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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by concretevitamin <gi...@git.apache.org>.
Github user concretevitamin commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47717946
Sorry, I didn't realize Reynold had already commented on this thread. The current changes with Option look good.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48015592
Thanks. I'm merging this in master & branch-1.0.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-47624045
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.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/1272#issuecomment-48010516
Merged build started.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14391324
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +65,21 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
- throw new NoSuchElementException(key)
- }
settings.get(key)
}
def get(key: String, defaultValue: String): String = {
- if (!settings.containsKey(key)) defaultValue else settings.get(key)
+ settings.synchronized {
+ if (!settings.containsKey(key)) defaultValue else settings.get(key)
--- End diff --
Let's use the ConcurrentHashMap-safe paradigm of
```scala
Option(settings.get(key)).getOrElse(defaultValue)
```
Note that ConcurrentHashMap does not allow null values, so this is safe.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14391342
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +65,21 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
- throw new NoSuchElementException(key)
- }
settings.get(key)
}
def get(key: String, defaultValue: String): String = {
- if (!settings.containsKey(key)) defaultValue else settings.get(key)
+ settings.synchronized {
+ if (!settings.containsKey(key)) defaultValue else settings.get(key)
+ }
}
def getAll: Array[(String, String)] = settings.asScala.toArray
def getOption(key: String): Option[String] = {
- if (!settings.containsKey(key)) None else Some(settings.get(key))
+ settings.synchronized {
+ if (!settings.containsKey(key)) None else Some(settings.get(key))
--- End diff --
Similarly, here, just
```scala
Option(settings.get(key))
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] spark pull request: Update SQLConf.scala
Posted by aarondav <gi...@git.apache.org>.
Github user aarondav commented on a diff in the pull request:
https://github.com/apache/spark/pull/1272#discussion_r14391366
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/SQLConf.scala ---
@@ -64,20 +65,21 @@ trait SQLConf {
}
def get(key: String): String = {
- if (!settings.containsKey(key)) {
- throw new NoSuchElementException(key)
- }
settings.get(key)
}
def get(key: String, defaultValue: String): String = {
- if (!settings.containsKey(key)) defaultValue else settings.get(key)
+ settings.synchronized {
+ if (!settings.containsKey(key)) defaultValue else settings.get(key)
--- End diff --
(Note that switching to this allows us to avoid adding the synchronized {} blocks.)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---