You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by cloud-fan <gi...@git.apache.org> on 2015/04/08 10:48:41 UTC
[GitHub] spark pull request: [Core][minor] remove unnecessary `parents` in ...
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/5416
[Core][minor] remove unnecessary `parents` in `ShuffleMapStage`
We never call `parents` on `ShuffleMapStage`, so we should define it only in `ResultStage`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark minor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/5416.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 #5416
----
commit 8871a58f9fc4ac054152e12f6930fc00ccf7f8c7
Author: Wenchen Fan <cl...@outlook.com>
Date: 2015-04-08T08:45:13Z
remove unnecessary parents variable
----
---
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: [Core][minor] remove unnecessary `parents` in ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan closed the pull request at:
https://github.com/apache/spark/pull/5416
---
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: [Core][minor] remove unnecessary `parents` in ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/5416#issuecomment-93177863
Yes, the parent of the second shuffle map stage will be the first shuffle map stage. Logically every `Stage` except the first one have parents, but the problem is we always re-calculate it when we need parent stages. So I think maybe we should change `parents` from a value into a method.
---
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: [Core][minor] remove unnecessary `parents` in ...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5416#issuecomment-90847067
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: [Core][minor] remove unnecessary `parents` in ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/5416#issuecomment-93282893
@srowen yes, I have changed my mind :)
Closing it and will open a small JIRA later.
---
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: [Core][minor] remove unnecessary `parents` in ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5416#issuecomment-90850089
I don't know this code well, but I suppose the question is, does it logically belong in `Stage` even if it only happens to be used in one subclass now? a stage seems to generally-speaking have parents.
---
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: [Core][minor] remove unnecessary `parents` in ...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/5416#issuecomment-93279092
@cloud-fan this seems like a different proposition though, not just removing an unused member. What does making it a method accomplish? It seems like you're suggesting reusing the existing value in more places, that's all. Once it's clear if there should be a change like this, maybe you can open a small JIRA and a new PR (or reuse this one), but either way this particular change isn't to be merged right?
---
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: [Core][minor] remove unnecessary `parents` in ...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/5416#issuecomment-90863451
The only place that use `parents` is [here](https://github.com/apache/spark/blob/master/core/src/main/scala/org/apache/spark/scheduler/DAGScheduler.scala#L756-759). I agree a stage generally-speaking have parents and `parents` belongs to `Stage` logically, but when we need parent stages, we always re-calculate it. The code I mentioned before is the only place we use the "cached" parent stages. Should we put `getParentStages` and some related methods into `Stage` class? @JoshRosen
---
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: [Core][minor] remove unnecessary `parents` in ...
Posted by andrewor14 <gi...@git.apache.org>.
Github user andrewor14 commented on the pull request:
https://github.com/apache/spark/pull/5416#issuecomment-93061196
@cloud-fan @JoshRosen what happens if I do a back-to-back shuffle like:
```
sc.parallelize(1 to 100).map { i => (i, i) }.groupByKey(...).reduceByKey(...).count()
```
My understanding is that this will run a shuffle map stage, then another shuffle map stage, then a reduce stage. It seems to me that the parent of the second shuffle map stage will be the first shuffle map stage, no?
---
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