You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rxin <gi...@git.apache.org> on 2016/05/02 06:40:07 UTC
[GitHub] spark pull request: [SPARK-15055] Remove setValue method on accumu...
GitHub user rxin opened a pull request:
https://github.com/apache/spark/pull/12837
[SPARK-15055] Remove setValue method on accumulators
## What changes were proposed in this pull request?
This patch removes setValue methods on the new accumulator API. Even though they were defined as private[spark], they can still be accessed in Java.
## How was this patch tested?
Updated test cases to reflect this.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rxin/spark SPARK-15055
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12837.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 #12837
----
commit bb25d1a74165c10a13a1a5d0729886bc76466b60
Author: Reynold Xin <rx...@databricks.com>
Date: 2016-05-02T06:38:26Z
[SPARK-15055] Remove setValue method on accumulators
----
---
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-15055] Remove setValue method on accumu...
Posted by holdenk <gi...@git.apache.org>.
Github user holdenk commented on a diff in the pull request:
https://github.com/apache/spark/pull/12837#discussion_r61708740
--- Diff: core/src/main/scala/org/apache/spark/executor/TaskMetrics.scala ---
@@ -102,19 +102,20 @@ class TaskMetrics private[spark] () extends Serializable {
// Setters and increment-ers
private[spark] def setExecutorDeserializeTime(v: Long): Unit =
--- End diff --
Since these now all call add (and don't reset), should we consider perhaps renaming them (or calling reset before add)? They are internal so no harm in renaming.
---
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-15055] Remove setValue method on accumu...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216116994
cc @cloud-fan
---
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-15055] Remove setValue method on accumu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216133424
**[Test build #57510 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57510/consoleFull)** for PR 12837 at commit [`bb25d1a`](https://github.com/apache/spark/commit/bb25d1a74165c10a13a1a5d0729886bc76466b60).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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-15055] Remove setValue method on accumu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216423616
**[Test build #57578 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57578/consoleFull)** for PR 12837 at commit [`421cdc8`](https://github.com/apache/spark/commit/421cdc8d0c450a652c0c62baa830b2eae0cfaeef).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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-15055] Remove setValue method on accumu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216149633
Merged build finished. Test FAILed.
---
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-15055] Remove setValue method on accumu...
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/12837#discussion_r61834496
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -295,12 +295,12 @@ private[spark] class Executor(
// Deserialization happens in two parts: first, we deserialize a Task object, which
// includes the Partition. Second, Task.run() deserializes the RDD and function to be run.
- task.metrics.setExecutorDeserializeTime(
+ task.metrics.incExecutorDeserializeTime(
(taskStart - deserializeStartTime) + task.executorDeserializeTime)
// We need to subtract Task.run()'s deserialization time to avoid double-counting
- task.metrics.setExecutorRunTime((taskFinish - taskStart) - task.executorDeserializeTime)
- task.metrics.setJvmGCTime(computeTotalGcTime() - startGCTime)
- task.metrics.setResultSerializationTime(afterSerialization - beforeSerialization)
+ task.metrics.incExecutorRunTime((taskFinish - taskStart) - task.executorDeserializeTime)
--- End diff --
So we do need the `setValue` function here?
---
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-15055] Remove setValue method on accumu...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12837#discussion_r61830132
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -295,12 +295,12 @@ private[spark] class Executor(
// Deserialization happens in two parts: first, we deserialize a Task object, which
// includes the Partition. Second, Task.run() deserializes the RDD and function to be run.
- task.metrics.setExecutorDeserializeTime(
+ task.metrics.incExecutorDeserializeTime(
(taskStart - deserializeStartTime) + task.executorDeserializeTime)
// We need to subtract Task.run()'s deserialization time to avoid double-counting
- task.metrics.setExecutorRunTime((taskFinish - taskStart) - task.executorDeserializeTime)
- task.metrics.setJvmGCTime(computeTotalGcTime() - startGCTime)
- task.metrics.setResultSerializationTime(afterSerialization - beforeSerialization)
+ task.metrics.incExecutorRunTime((taskFinish - taskStart) - task.executorDeserializeTime)
--- End diff --
actually all of these are wrong ...
---
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-15055] Remove setValue method on accumu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216423677
Merged build finished. Test FAILed.
---
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-15055] Remove setValue method on accumu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216410942
**[Test build #57578 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57578/consoleFull)** for PR 12837 at commit [`421cdc8`](https://github.com/apache/spark/commit/421cdc8d0c450a652c0c62baa830b2eae0cfaeef).
---
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-15055] Remove setValue method on accumu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216121466
**[Test build #57516 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57516/consoleFull)** for PR 12837 at commit [`8d144c9`](https://github.com/apache/spark/commit/8d144c9bb12b15e230153bb53e195ccd140decce).
---
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-15055] Remove setValue method on accumu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216149637
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57516/
Test FAILed.
---
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-15055] Remove setValue method on accumu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216133837
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57510/
Test FAILed.
---
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-15055] Remove setValue method on accumu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216149193
**[Test build #57516 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57516/consoleFull)** for PR 12837 at commit [`8d144c9`](https://github.com/apache/spark/commit/8d144c9bb12b15e230153bb53e195ccd140decce).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
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-15055] Remove setValue method on accumu...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216117212
**[Test build #57510 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/57510/consoleFull)** for PR 12837 at commit [`bb25d1a`](https://github.com/apache/spark/commit/bb25d1a74165c10a13a1a5d0729886bc76466b60).
---
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-15055] Remove setValue method on accumu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216133834
Merged build finished. Test FAILed.
---
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-15055] Remove setValue method on accumu...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/12837#discussion_r61835738
--- Diff: core/src/main/scala/org/apache/spark/executor/Executor.scala ---
@@ -295,12 +295,12 @@ private[spark] class Executor(
// Deserialization happens in two parts: first, we deserialize a Task object, which
// includes the Partition. Second, Task.run() deserializes the RDD and function to be run.
- task.metrics.setExecutorDeserializeTime(
+ task.metrics.incExecutorDeserializeTime(
(taskStart - deserializeStartTime) + task.executorDeserializeTime)
// We need to subtract Task.run()'s deserialization time to avoid double-counting
- task.metrics.setExecutorRunTime((taskFinish - taskStart) - task.executorDeserializeTime)
- task.metrics.setJvmGCTime(computeTotalGcTime() - startGCTime)
- task.metrics.setResultSerializationTime(afterSerialization - beforeSerialization)
+ task.metrics.incExecutorRunTime((taskFinish - taskStart) - task.executorDeserializeTime)
--- End diff --
Yes. But I'd still remove the setValue, and just call reset().add(..), once we decouple copyAndReset into copy and reset.
---
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-15055] Remove setValue method on accumu...
Posted by rxin <gi...@git.apache.org>.
Github user rxin closed the pull request at:
https://github.com/apache/spark/pull/12837
---
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-15055] Remove setValue method on accumu...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12837#issuecomment-216423679
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/57578/
Test FAILed.
---
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