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/08/27 08:45:24 UTC
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
GitHub user baishuo opened a pull request:
https://github.com/apache/spark/pull/2157
[SPARK-3241][SQL] create NumberFormat instance by threadsafe way
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/baishuo/spark patch-threadlocal
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2157.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 #2157
----
commit 5d05a01d7737ee86ed42cb004b01d0cf22d4d695
Author: baishuo <vc...@hotmail.com>
Date: 2014-08-27T03:12:24Z
create NumberFormat instance by threadsafe way
----
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on a diff in the pull request:
https://github.com/apache/spark/pull/2157#discussion_r16765443
--- Diff: sql/hive/src/main/scala/org/apache/spark/SparkHadoopWriter.scala ---
@@ -71,7 +71,7 @@ private[hive] class SparkHiveHadoopWriter(
}
def open() {
- val numfmt = NumberFormat.getInstance()
+ val numfmt = SparkHiveHadoopWriter.threadLocalNumberFormat.get()
--- End diff --
This is a local variable, right? It's not shared with other threads. It is already creating a new instance for each call. Or if you argue it could be the same instance -- a `ThreadLocal` wouldn't help. But it really can't be a singleton.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/2157#issuecomment-53669323
yes,no problem :) close this issue
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo closed the pull request at:
https://github.com/apache/spark/pull/2157
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by marmbrus <gi...@git.apache.org>.
Github user marmbrus commented on the pull request:
https://github.com/apache/spark/pull/2157#issuecomment-53631226
Hi @baishuo, thanks for working on this. I agree with @chenghao-intel and @srowen I think this code was already thread safe and we don't really need the added complexity of a thread local variable as we are not allocating new NumberFormats in the critical path. Do you think you could Close this issue unless you disagree.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/2157#issuecomment-53546624
Sorry for commenting out of order -- I missed the original comment. @baishuo why do you want to avoid multiple instances per thread? that does not solve a thread-safety problem.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/2157#issuecomment-53532690
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by baishuo <gi...@git.apache.org>.
Github user baishuo commented on the pull request:
https://github.com/apache/spark/pull/2157#issuecomment-53544332
thank you @chenghao-intel . I think I didnt express what I think clearly. why there is a threadlocal is to ensure there is one and only one NumberFormat instance in the same thread. othrewise, if open was called more than once, there maybe more than one instance of NumberFormat
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3241][SQL] create NumberFormat instance...
Posted by chenghao-intel <gi...@git.apache.org>.
Github user chenghao-intel commented on the pull request:
https://github.com/apache/spark/pull/2157#issuecomment-53534767
I don't think we need to change anything. `NumberFormat` is not thread-safe mean we can not share the same `NumberFormat` instance among different threads (coz they may call the `parse` or `format` methods). Obviously, it's not the case here. And the `NumberFormat.getInstance()` itself should be thread-safe if I understand correctly.
---
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org