Skip to content

Conversation

@amits83
Copy link

@amits83 amits83 commented May 2, 2016

What changes were proposed in this pull request?

Removal of SPARK_YARN_USER_ENV env variable use in the code

How was this patch tested?

Ran ./dev/lint-scala to verify changes.

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@vanzin
Copy link
Contributor

vanzin commented May 2, 2016

Hi @amits83 the bug is about removal of all env-based configuration, not just this single one. Have you looked at the original bug description, or any of the other env variables used in the code? There's a lot more work to achieve the goal of the bug.

@amits83
Copy link
Author

amits83 commented May 2, 2016

Hi @vanzin , yes I know that.

This was my first commit so limited it to just one var at this time. :)

I was looking at conf/spark-env.sh.template . Do you think thats a good starting point to find env variables listed in the template within the code and start removing them?

@vanzin
Copy link
Contributor

vanzin commented May 2, 2016

@amits83 yes that file looks like a good place to start. Especially because a bunch of those have already been removed.

If you don't plan to fix the whole bug, the usual approach is to open a sub-task for the specific thing you're fixing. In this case though, it would be better to avoid a bunch of really tiny PRs and take a more holistic look at the problem, since a lot of them will require the functionality that I described in the bug.

@amits83
Copy link
Author

amits83 commented May 2, 2016

@vanzin thanks for that. I dont intend to take a piecemeal approach.

I will work on the other vars and file a PR for all of them together.

Thanks again.
On May 2, 2016 1:52 PM, "Marcelo Vanzin" [email protected] wrote:

@amits83 https://github.com/amits83 yes that file looks like a good
place to start. Especially because a bunch of those have already been
removed.

If you don't plan to fix the whole bug, the usual approach is to open a
sub-task for the specific thing you're fixing. In this case though, it
would be better to avoid a bunch of really tiny PRs and take a more
holistic look at the problem, since a lot of them will require the
functionality that I described in the bug.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#12848 (comment)

@vanzin
Copy link
Contributor

vanzin commented Jun 8, 2016

Hi @amits83, were you planning to also look at the other env variables? Otherwise should probably close this for the time being.

@amits83
Copy link
Author

amits83 commented Jun 10, 2016

Hi @vanzin yes I'm working on this.

I intend to send in a PR soon.

On Wed, Jun 8, 2016 at 4:31 PM, Marcelo Vanzin [email protected]
wrote:

Hi @amits83 https://github.com/amits83, were you planning to also look
at the other env variables? Otherwise should probably close this for the
time being.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#12848 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AJbzSsjLyauFURinzikwm-kqSk2bYaXTks5qJ1DEgaJpZM4IVrUx
.

With Best Regards,
Amit Shinde.

@vanzin
Copy link
Contributor

vanzin commented Jul 12, 2016

@amits83 could you close this PR if you're not going to address the comments? thanks

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.

3 participants