Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Spark-14686] Allow setting local properties that are not inheritable #12456

Closed
wants to merge 4 commits into from

Conversation

marcintustin
Copy link
Contributor

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.

@marcintustin
Copy link
Contributor Author

I should add that this will also need new tests. I haven't added any, again pending overall agreement on design.

@rxin
Copy link
Contributor

rxin commented Apr 17, 2016

Can you add a more descriptive title?

@rxin
Copy link
Contributor

rxin commented Apr 17, 2016

The title should probably just be

"[SPARK-14686] Allow setting local properties that are not inheritable"

@marcintustin
Copy link
Contributor Author

@rxin Derp on my part. Of course this needs a better title.

@marcintustin marcintustin changed the title [Spark-14686][CORE,SQL,STREAMING] [Spark-14686] Allow setting local properties that are not inheritable Apr 17, 2016
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@rxin
Copy link
Contributor

rxin commented Apr 18, 2016

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?

@jerryshao
Copy link
Contributor

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 (mesos/spark#937).

If there's other solution to solve this issue, I fully agree with the change to non-inheritable variables.

@marcintustin
Copy link
Contributor Author

@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.

@jerryshao
Copy link
Contributor

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.

@mtustin-handy
Copy link
Contributor

@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?

@HyukjinKwon
Copy link
Member

Hi @marcintustin and all, where are we on this? is this still active?

@marcintustin
Copy link
Contributor Author

marcintustin commented Jun 2, 2017 via email

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Jun 2, 2017

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 for now? If it is the latter case, I think we could leave this closed for now. We can reopen this whenever you have some time.

@marcintustin
Copy link
Contributor Author

marcintustin commented Jun 2, 2017 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants