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 2014/09/03 09:02:32 UTC
[GitHub] spark pull request: [SPARK-3367] Remove spark.shuffle.spill.compre...
GitHub user rxin opened a pull request:
https://github.com/apache/spark/pull/2247
[SPARK-3367] Remove spark.shuffle.spill.compress
Replaced it with existing spark.shuffle.compress.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rxin/spark shuffle.spill.compress
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/2247.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 #2247
----
commit 2029d60569036a83475b10cafa51a64a74c4cf9f
Author: Reynold Xin <rx...@apache.org>
Date: 2014-09-03T07:01:58Z
[SPARK-3367] Remove spark.shuffle.spill.compress
Replaced it with existing spark.shuffle.compress.
----
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54264700
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.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request: [SPARK-3367] Remove spark.shuffle.spill.compre...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54267869
Yes, exactly : with higher block size configured sometimes.
Most tasks will not incur (de)compression overhead except for the very few
which spill due to skew.
On 03-Sep-2014 2:09 pm, "Reynold Xin" <no...@github.com> wrote:
> I don't get it. Are you saying compression is turned on for spill files,
> but not for general shuffle files?
>
> And in response to your original question about sort leakage: yes this is
> breaking that. See discussion in #2178 (comment)
> <https://github.com/apache/spark/pull/2178#issuecomment-54214760>
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2247#issuecomment-54267261>.
>
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54377008
I haven't looked at all the details of this, but to me configs are public interfaces. By you removing one that someone had used in the past could now cause their program to behave differently between minor versions, which might be unexpected. In general I would try to keep things backwards compatible. If this is direct mapping why not just have the code check if either is set?
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54266944
The overhead of compressing ?
On 03-Sep-2014 2:02 pm, "Reynold Xin" <no...@github.com> wrote:
> What compression overhead are you talking about?
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2247#issuecomment-54266574>.
>
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54268199
Do you have any data on a job in which disabling shuffle compression helps with performance substantially (especially with sort-based shuffle in which the number of concurrent streams is low)? In all cases I've seen, compression improves performance. I'm not sure if we should keep two options to satisfy an extremely rare corner case.
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54360218
Yes, unfortunately we currently document `spark.shuffle.spill.compress` in addition to `spark.shuffle.compress`
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54521058
Sure, I'd be happy to codify that. In general my approach to this has been that we keep all config settings and defaults that affect correctness (whether the program gives the right result), but things that affect performance are fair to change. For example, we've kept spark.serializer to be Java since changing this by default would break a lot of apps. But we've tweaked things like the % of memory used for caching and buffer sizes, and we've also made some settings largely irrelevant (e.g. Akka frame size used to matter a lot, but we moved first task results and then tasks themselves to not be sent through Akka).
Anyway, in this case I'm not arguing that this should be removed in 1.X if there are concerns, I'm just trying to understand whether this is a useful setting to have longer-term. In particular, should we deprecate it and remove it in 2.X? So far I haven't seen a strong case that this is a useful setting, but I might be missing some use case.
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54266574
What compression overhead are you talking about?
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54272557
I think we are looking at it the wrong way - this is an exposed interface
which is being removed in a minor release.
It might also break direct usage of ExternalAppendOnlyMap, but since that
is developerapi, that is less of direct concern (though potentially more
disruptive).
On 03-Sep-2014 2:19 pm, "Reynold Xin" <no...@github.com> wrote:
> Do you have any data on a job in which disabling shuffle compression helps
> with performance substantially (especially with sort-based shuffle in which
> the number of concurrent streams is low)? In all cases I've seen,
> compression improves performance. I'm not sure if we should keep two
> options to satisfy an extremely rare corner case.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2247#issuecomment-54268199>.
>
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54267261
I don't get it. Are you saying compression is turned on for spill files, but not for general shuffle files?
And in response to your original question about sort leakage: yes this is breaking that. See discussion in https://github.com/apache/spark/pull/2178#issuecomment-54214760
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54266318
Is this being done to workaround the bug (sort shuffle breakage) ?
The way we use it is, not use compress but only enable it if spilled to
disk (a corner case) - minimizing compression overhead for normal case
while enabling handling for degenerate cases.
On 03-Sep-2014 1:49 pm, "Apache Spark QA" <no...@github.com> wrote:
> QA tests have started
> <https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19651/consoleFull>
> for PR 2247 at commit 2029d60
> <https://github.com/apache/spark/commit/2029d60569036a83475b10cafa51a64a74c4cf9f>
> .
>
> - This patch merges cleanly.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2247#issuecomment-54265330>.
>
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54264487
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19643/consoleFull) for PR 2247 at commit [`2029d60`](https://github.com/apache/spark/commit/2029d60569036a83475b10cafa51a64a74c4cf9f).
* This patch **fails** 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-3367] Remove spark.shuffle.spill.compre...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54265330
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19651/consoleFull) for PR 2247 at commit [`2029d60`](https://github.com/apache/spark/commit/2029d60569036a83475b10cafa51a64a74c4cf9f).
* This patch merges cleanly.
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54362759
It's not an interface in the sense that program output doesn't change. It's only a config setting for an optimization.
Anyway, I agree with Reynold -- I'd like to see an example where spark.shuffle.spill.compress and spark.shuffle.compress need to be at different values, and see performance numbers for that. It seems to me that you're spilling the same kind of objects in both, so there will be the same tradeoff between I/O and compute time.
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54723134
Ok. I have filed a JIRA at https://issues.apache.org/jira/browse/SPARK-3426
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by mridulm <gi...@git.apache.org>.
Github user mridulm commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54395088
I am not sure I follow ... We compress shuffle with first while we compress
spills with second. How would that be same ? The perf characteristics would
be different.
The team has decided to use it, and I don't want to second guess their
reasons whatever they might be (that just goes into unnecessary
discussions).
Given it is a public interface available in 1.0 (iirc since much before ?)
, we can't remove support for it until next major release.... We can of
course deprecate and warn against use, but will need to continue supporting
it until next major release.
On 04-Sep-2014 2:10 am, "Matei Zaharia" <no...@github.com> wrote:
> It's not an interface in the sense that program output doesn't change.
> It's only a config setting for an optimization.
>
> Anyway, I agree with Reynold -- I'd like to see an example where
> spark.shuffle.spill.compress and spark.shuffle.compress need to be at
> different values, and see performance numbers for that. It seems to me that
> you're spilling the same kind of objects in both, so there will be the same
> tradeoff between I/O and compute time.
>
> —
> Reply to this email directly or view it on GitHub
> <https://github.com/apache/spark/pull/2247#issuecomment-54362759>.
>
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by rxin <gi...@git.apache.org>.
Github user rxin closed the pull request at:
https://github.com/apache/spark/pull/2247
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54259798
[QA tests have started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19643/consoleFull) for PR 2247 at commit [`2029d60`](https://github.com/apache/spark/commit/2029d60569036a83475b10cafa51a64a74c4cf9f).
* This patch merges cleanly.
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by mateiz <gi...@git.apache.org>.
Github user mateiz commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54399595
The same data that gets spilled gets shuffled in most operations. Anyway, I'd be okay keeping it, but I'd like to understand why it's important. In general we should be aiming to reduce the number of config settings. Maybe this team is doing something wrong by using it. It's important to know the reason if we want to make a decision about reducing the # of config settings.
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54703802
Ok I'm going to close this for now given the debate.
@mridulm it would be great to hear more about the performance difference. It is hard for me to imagine a scenario in which you would want different settings for the two.
@andrewor14 can you fix the bug?
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by tgravescs <gi...@git.apache.org>.
Github user tgravescs commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54474045
Perhaps we should have a separate discussion on the dev mailing list as what we consider public interfaces? That way everyone is in agreement or at least is following the same rules when things get committed.
---
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-3367] Remove spark.shuffle.spill.compre...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/2247#issuecomment-54271426
[QA tests have finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/19651/consoleFull) for PR 2247 at commit [`2029d60`](https://github.com/apache/spark/commit/2029d60569036a83475b10cafa51a64a74c4cf9f).
* This patch **passes** 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