You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by sourind <gi...@git.apache.org> on 2016/01/26 01:24:31 UTC
[GitHub] spark pull request: [SPARK-12958] [CORE] Adding Map accumulator St...
GitHub user sourind opened a pull request:
https://github.com/apache/spark/pull/10907
[SPARK-12958] [CORE] Adding Map accumulator String->Long
Users can use this to accumulate long values against String keys
@srowen Can you please give permission for testing it in Jenkins ?
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/sourind/spark map_accum
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/10907.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 #10907
----
commit ebe9342a2342a02df1ecdfb3a96c71b15939a63e
Author: sourind <so...@yahoo-inc.com>
Date: 2016-01-22T18:29:28Z
Adding Map accumulator String->Long
----
---
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-12958] [CORE] Adding Map accumulator St...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-175335903
I'm against this in its current form; this seems like a niche feature for which there's not high demand.
---
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-12958] [CORE] Adding Map accumulator St...
Posted by sourind <gi...@git.apache.org>.
Github user sourind commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-178224859
Sure.
--Souri
On Mon, Feb 1, 2016 at 2:00 PM, andrewor14 <no...@github.com> wrote:
> I agree with @JoshRosen <https://github.com/JoshRosen>. It seems this map
> param is essentially a collection of a few individual Long accums, and the
> user can manage that better themselves. Also, the user can always write
> their own map param if they want.
>
> @sourind <https://github.com/sourind> let's close this PR.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/10907#issuecomment-178216544>.
>
---
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-12958] [CORE] Adding Map accumulator St...
Posted by sourind <gi...@git.apache.org>.
Github user sourind closed the pull request at:
https://github.com/apache/spark/pull/10907
---
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-12958] [CORE] Adding Map accumulator St...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-178528937
@sourind please close this pull request
---
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-12958] [CORE] Adding Map accumulator St...
Posted by jodersky <gi...@git.apache.org>.
Github user jodersky commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-174759131
Just saw the discussion on the JIRA.
IMO this is a bit too specific, wouldn't it make sense to include a more generic version of MapAccumulatorParam. Something like MapAccumulatorParam[A, B], where B has to have an implicit AccumulatorParam[B] itself?
---
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-12958] [CORE] Adding Map accumulator St...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-178216544
I agree with @JoshRosen. It seems this map param is essentially a collection of a few individual Long accums, and the user can manage that better themselves. Also, the user can always write their own map param if they want.
@sourind let's close this PR.
---
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-12958] [CORE] Adding Map accumulator St...
Posted by sourind <gi...@git.apache.org>.
Github user sourind commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-174761380
Thanks for looking into it. Most of the time, string keys should solve the
problem but yes, no harm in making it more generic. I will work on it and
submit a new patch.
On Mon, Jan 25, 2016 at 5:30 PM, Jakob Odersky <no...@github.com>
wrote:
> Just saw the discussion on the JIRA.
> IMO this is a bit too specific, wouldn't it make sense to include a more
> generic version of MapAccumulatorParam. Something like
> MapAccumulatorParam[A, B], where B has to have an implicit
> AccumulatorParam[B] itself?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/10907#issuecomment-174759131>.
>
---
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-12958] [CORE] Adding Map accumulator St...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-174773667
Do we need this in Spark? It seems like it is just a few lines of code.
---
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-12958] [CORE] Adding Map accumulator St...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-174745493
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-12958] [CORE] Adding Map accumulator St...
Posted by sourind <gi...@git.apache.org>.
Github user sourind commented on the pull request:
https://github.com/apache/spark/pull/10907#issuecomment-175187527
There is a TODO in Accumulators.scala
<https://github.com/apache/spark/blob/302bb569f3e1f09e2e7cea5e4e7f5c6953b0fc82/core/src/main/scala/org/apache/spark/Accumulator.scala#L159>
for
adding different accumulator params (Lists, Strings etc.). This change is
similar to that.
Also, with this change people can accumulate values for keys that are not
initially defined in the driver -therefore, allowing users to define
metrics at runtime.
Let me know your thoughts on this.
Thanks,
Souri
On Mon, Jan 25, 2016 at 6:00 PM, Reynold Xin <no...@github.com>
wrote:
> Do we need this in Spark? It seems like it is just a few lines of code.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/10907#issuecomment-174773667>.
>
---
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