You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by marcintustin <gi...@git.apache.org> on 2016/04/17 20:40:44 UTC
[GitHub] spark pull request: [Spark-14686][CORE,SQL,STREAMING]
GitHub user marcintustin opened a pull request:
https://github.com/apache/spark/pull/12456
[Spark-14686][CORE,SQL,STREAMING]
## What changes were proposed in this pull request?
This PR adds a uninheritableLocalPropertyFacility and ports sql.execution.id to be set with that facility.
If this is to go forward, the changes should probably be folded into a Properties type which accommodates hierarchical access rather than a tuple.
## How was this patch tested?
Running tests.
@rxin @JoshRosen PR opened for comments. As noted above, this should probably have a little more engineering done, but I'd like to (a) get feedback on the overall approach; and (b) see which tests fail in jenkins, as I have some tests failing locally which may or may not be bogus.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/marcintustin/spark SPARK-14686
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/12456.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 #12456
----
commit aecb305577a7d16065738afcf4bbeee6397b4f53
Author: Marcin Tustin <ma...@gmail.com>
Date: 2016-04-16T23:41:22Z
[SPARK-14685] Document inheritability of localProperties
commit 9964e2e937718194521c6705ba345deba11f1f3d
Author: Marcin Tustin <ma...@gmail.com>
Date: 2016-04-17T14:49:51Z
Add test for heritability of local properties
commit b96cde118c1265bf37ac7036581b8bb1bef80ee0
Author: Marcin Tustin <ma...@gmail.com>
Date: 2016-04-17T15:09:38Z
SPARK-14685 add test to ensure no crosstalk between threads on localProperties
Work with me in NYC: https://www.handy.com/careers/73115?gh_jid=73115&gh_src=o5qcxn
commit bc302e014f280cca9d9b7f104c14cdf7de6d5df4
Author: Marcin Tustin <ma...@gmail.com>
Date: 2016-04-17T18:35:36Z
[SPARK-14686] First cut of non-inheritable localProperties
----
---
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 issue #12456: [Spark-14686] Allow setting local properties that are no...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/12456
Hi @marcintustin and all, where are we on this? is this still active?
---
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-14686] Allow setting local properties t...
Posted by mtustin-handy <gi...@git.apache.org>.
Github user mtustin-handy commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-213037791
@jerryshao Makes sense. Would having an explicitly inheritable facility, or a way to switch the spark context into inheritable mode (whatever that design looks like - e.g. a subclass or facade object) meet the need?
---
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-14686][CORE,SQL,STREAMING]
Posted by marcintustin <gi...@git.apache.org>.
Github user marcintustin commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-211080503
@rxin Derp on my part. Of course this needs a better title.
---
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-14686][CORE,SQL,STREAMING]
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-211079376
Can you add a more descriptive title?
---
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-14686] Allow setting local properties t...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-211487324
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 #12456: [Spark-14686] Allow setting local properties that...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/12456
---
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-14686] Allow setting local properties t...
Posted by marcintustin <gi...@git.apache.org>.
Github user marcintustin commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-212672809
@jerryshao
> there's no way for user to set it in the job thread specifically,
Can you explain what you mean by that? Or link us to some code? It's totally possible that it is impossible, or that it's possible.
---
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 issue #12456: [Spark-14686] Allow setting local properties that are no...
Posted by marcintustin <gi...@git.apache.org>.
Github user marcintustin commented on the issue:
https://github.com/apache/spark/pull/12456
Feel free to close
--
Marcin Tustin
Tel: +1 917 553 3974
---
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-14686] Allow setting local properties t...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-211700705
Hi @rxin , the reason to use inheritable variable is for some scenarios like Spark Streaming + FIFO scheduling, the property of pool is set in parent thread (main thread) and be picked in child thread (job thread). Basically if the creation of `SparkContext` and submitting job are in two threads, local properties should be passed from parent thread to child thread. IIUC, there's no way for user to set it in the job thread specifically, so that's why I changed long time ago, here is the original PR (https://github.com/mesos/spark/pull/937).
If there's other solution to solve this issue, I fully agree with the change to non-inheritable variables.
---
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-14686] Allow setting local properties t...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-211505486
cc @jerryshao
I just talked to @JoshRosen more and thinking whether we should make this always non-inheritable. Josh said Jerry might've added something in the past that relied on this. @jerryshao can you comment on whether it would be a problem if we make this always non-inheritable and always explicitly pass properties over in spawned threads?
---
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-14686][CORE,SQL,STREAMING]
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-211079561
The title should probably just be
"[SPARK-14686] Allow setting local properties that are not inheritable"
---
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 issue #12456: [Spark-14686] Allow setting local properties that are no...
Posted by marcintustin <gi...@git.apache.org>.
Github user marcintustin commented on the issue:
https://github.com/apache/spark/pull/12456
I am not actively working on this.
--
Marcin Tustin
Tel: +1 917 553 3974
---
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-14686] Allow setting local properties t...
Posted by jerryshao <gi...@git.apache.org>.
Github user jerryshao commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-212677776
For example, if you have a main thread to create a `SparkContext`, and a child thread to use this `SparkContext` to create `StreamingContext` and set local property. Now if you have some streaming logics, in each batch duration these streaming logics will be translated into RDD operations and submitted to `SparkContext` in another job thread. So if local property is not inheritable, the property picked by this job thread will be empty, unless you manually copy the property from the thread you set the local property. For a user there's no chance to do it. But if you will change the Spark code, I think there's always a way to handle it.
---
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-14686][CORE,SQL,STREAMING]
Posted by marcintustin <gi...@git.apache.org>.
Github user marcintustin commented on the pull request:
https://github.com/apache/spark/pull/12456#issuecomment-211078297
I should add that this will also need new tests. I haven't added any, again pending overall agreement on design.
---
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 issue #12456: [Spark-14686] Allow setting local properties that are no...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/12456
I just want to be sure. Is it waiting for reviewer's comment or you just happened to don't have some time to keep this up-to-date? If it is the latter case, I think we could leave this closed for now. We can reopen this whenever you have some 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