Skip to content

Conversation

@kevincox
Copy link
Contributor

@kevincox kevincox commented Oct 6, 2015

Currently if it isn't set it scans /lib/* and adds every dir to the
classpath which makes the env too large and every command called
afterwords fails.

@kevincox kevincox changed the title Only add hive to classpath if HIVE_HOME is set. [SPARK-10952] Only add hive to classpath if HIVE_HOME is set. Oct 6, 2015
@JoshRosen
Copy link
Contributor

Jenkins, this is ok to test.

@SparkQA
Copy link

SparkQA commented Oct 6, 2015

Test build #43277 has finished for PR 8994 at commit 37c02b0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

build/sbt Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a bash expert, but should this be using something like if [ -d "$HIVE_HOME" ]; to check that it's actually a directory and exists, similar to how we check for the existence of the assembly directory in

if [ -d "$ASSEMBLY_DIR" ]; then
?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't know. I was thinking "If you set the variable we will try to use it" plus if isn't correct the wild card will expand to nothing anyways. The other option is to warn if HIVE_HOME is set but doesn't point to a directory. I'm not too convinced either way but the way it is now makes sense to me. Although the warning won't hurt.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I only commented because I'm actually somewhat wary of bash-isms and http://unix.stackexchange.com/a/32211/42294 came to mind.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'm good here because it is in quotes so it will always be an argument. However there is actually a problem if there is a leading - in the variable so I will update to prevent that.

@JoshRosen
Copy link
Contributor

Minor question aside, this LGTM.

@kevincox
Copy link
Contributor Author

kevincox commented Oct 6, 2015

If you would prefer the change I can do it, just let me know.

Currently if it isn't set it scans `/lib/*` and adds every dir to the
classpath which makes the env too large and every command called
afterwords fails.
@kevincox kevincox force-pushed the kevincox-only-add-hive-to-classpath-if-var-is-set branch from 37c02b0 to 72b88b2 Compare October 6, 2015 23:28
@SparkQA
Copy link

SparkQA commented Oct 7, 2015

Test build #43305 has finished for PR 8994 at commit 72b88b2.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like these lines might be indented using using tabs instead of spaces; I'll just fix this myself on merge.

@JoshRosen
Copy link
Contributor

LGTM now, so I'm going to merge this to master and branch-1.5.

asfgit pushed a commit that referenced this pull request Oct 7, 2015
Currently if it isn't set it scans `/lib/*` and adds every dir to the
classpath which makes the env too large and every command called
afterwords fails.

Author: Kevin Cox <[email protected]>

Closes #8994 from kevincox/kevincox-only-add-hive-to-classpath-if-var-is-set.
@asfgit asfgit closed this in 9672602 Oct 7, 2015
asfgit pushed a commit that referenced this pull request Oct 7, 2015
Currently if it isn't set it scans `/lib/*` and adds every dir to the
classpath which makes the env too large and every command called
afterwords fails.

Author: Kevin Cox <[email protected]>

Closes #8994 from kevincox/kevincox-only-add-hive-to-classpath-if-var-is-set.
@kevincox kevincox deleted the kevincox-only-add-hive-to-classpath-if-var-is-set branch October 26, 2015 17:19
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