Skip to content

Add HADOOP_CLIENT_LIB_DIR to accumulo-env.sh - #6531

Open
lbschanno wants to merge 1 commit into
apache:mainfrom
lbschanno:hadoop-client-lib-dir
Open

lbschanno wants to merge 1 commit into
apache:mainfrom
lbschanno:hadoop-client-lib-dir

Conversation

@lbschanno

Copy link
Copy Markdown
Contributor

The accumulo-env.sh attempts to add Hadoop libraries to the classpath from the path $HADOOP_HOME/share/hadoop/client. There are times when Hadoop libraries are not installed in the tarball-style path, such as when Hadoop is installed via RPMs.

Add the environment variable HADOOP_CLIENT_LIB_DIR to accumulo-env with a default value of $HADOOP_HOME/share/hadoop/client. Update the construction of the CLASSPATH to add hadoop libraries using the HADOOP_CLIENT_LIB_DIR variable.

Closes #6530

The accumulo-env.sh attempts to add Hadoop libraries to the classpath
from the path $HADOOP_HOME/share/hadoop/client. There are times when
Hadoop libraries are not installed in the tarball-style path, such as
when Hadoop is installed via RPMs.

Add the environment variable HADOOP_CLIENT_LIB_DIR to accumulo-env with
a default value of $HADOOP_HOME/share/hadoop/client. Update the
construction of the CLASSPATH to add hadoop libraries using the
HADOOP_CLIENT_LIB_DIR variable.

Closes apache#6530
# lib is set by calling script that sources this env file
#shellcheck disable=SC2154
CLASSPATH="${CLASSPATH}:${lib}/*:${HADOOP_CONF_DIR}:${ZOOKEEPER_HOME}/*:${ZK_JARS}:${HADOOP_HOME}/share/hadoop/client/*"
CLASSPATH="${CLASSPATH}:${lib}/*:${HADOOP_CONF_DIR}:${ZOOKEEPER_HOME}/*:${ZK_JARS}:${HADOOP_CLIENT_LIB_DIR}/*"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looking at lines starting at 58, the intention is to honor the CLASSPATH environment variable set by the user and put that first.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This change does not circumnavigate that intention.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

After re-reading this, I think you were trying to tell me (correct me if I'm wrong) that I could configure the CLASSPATH variable beforehand to point to a custom hadoop lib directory. While this is true, I do think there is still some value to extracting the partially hardcoded path to a customizable variable.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

After re-reading this, I think you were trying to tell me (correct me if I'm wrong) that I could configure the CLASSPATH variable beforehand to point to a custom hadoop lib directory.

Yes, that's correct. For 4.0 this is the preferred method.

While this is true, I do think there is still some value to extracting the partially hardcoded path to a customizable variable.

I agree that the default directory may not be correct for users. But in my opinion I think making another variable that users can override might make it confusing for users because now they have several options, instead of one, for configuring the classpath (and the CLASSPATH variable is the well known standard).

Users that were taking advantage of prior classloading capabilities (VFS, lib/ext reloading, etc.) will need to change their configurations as those things have been removed.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing I dislike about this change is adding a new built-in environment variable that the default out-of-the-box reference env script uses makes things more complicated. There would be more possible paths to achieve the desired classpath and users may come to expect our reference env script that uses one of these paths, rather than feel obligated to tailor their own env script for their deployment, as intended.

Two things I like about this change are:

  1. that it standardizes the env variables a bit more in the shipped reference env script, so there's one for the config and one for the jars, and
  2. it's clear that other content in HADOOP_HOME are actually not needed or used.

Note: regardless of these changes, I think ${HADOOP_CONF_DIR} probably should be relocated next to the hadoop jar portion. It's a little weird that those are split up by the ZK stuff.

@ctubbsii ctubbsii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not entirely opposed to this change, but I'm not sure this change is really necessary. I share @dlmarion 's concern about adding complexity to what is essentially a reference script. Users are expected to tailor their environment configuration script for their environment. The one that we ship by default is just an example or reference. It would be unusual if it was suitable for every use case, and trying to make it support more use cases is out of scope.

I think it's valid to question whether the one we ship is a suitable default reference, but I think for me to really get behind a change like this, I would probably prefer to see this replace the HADOOP_HOME variable, rather than add to it. I'd also want to see it done in a way that is consistent with the way the ZK jars are added to the CLASSPATH, and right now, those seem to be done in two different ways.

The goal is ultimately a minimally complex reference that can work out of the box. I think the question to ask is: what should the minimal out-of-the-box env script be? I was thinking... if I were to package Accumulo/Hadoop/ZK as an RPM again, as I've done in the past for Fedora/EPEL, I would just use build-classpath/xmvn-resolve for the CLASSPATH, and put the other environment directly in the systemd unit files. I probably wouldn't even use this file for any environment. But our minimally complex reference shouldn't assume any of that. If we can support more use cases through simplicity and reduced assumptions, I think that's probably better than supporting use cases through added environment variables and complexity. Our current reference file is probably already more complex than it needs to be.

Comment on lines +39 to +40
## Hadoop client jars location.
HADOOP_CLIENT_LIB_DIR="${HADOOP_CLIENT_LIB_DIR:-${HADOOP_HOME}/share/hadoop/client}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This change effectively makes the HADOOP_HOME environment variable inconsequential on line 36, and the subsequent checks around line 53 don't make sense anymore.

# lib is set by calling script that sources this env file
#shellcheck disable=SC2154
CLASSPATH="${CLASSPATH}:${lib}/*:${HADOOP_CONF_DIR}:${ZOOKEEPER_HOME}/*:${ZK_JARS}:${HADOOP_HOME}/share/hadoop/client/*"
CLASSPATH="${CLASSPATH}:${lib}/*:${HADOOP_CONF_DIR}:${ZOOKEEPER_HOME}/*:${ZK_JARS}:${HADOOP_CLIENT_LIB_DIR}/*"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

One thing I dislike about this change is adding a new built-in environment variable that the default out-of-the-box reference env script uses makes things more complicated. There would be more possible paths to achieve the desired classpath and users may come to expect our reference env script that uses one of these paths, rather than feel obligated to tailor their own env script for their deployment, as intended.

Two things I like about this change are:

  1. that it standardizes the env variables a bit more in the shipped reference env script, so there's one for the config and one for the jars, and
  2. it's clear that other content in HADOOP_HOME are actually not needed or used.

Note: regardless of these changes, I think ${HADOOP_CONF_DIR} probably should be relocated next to the hadoop jar portion. It's a little weird that those are split up by the ZK stuff.

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.

Allow alternative path to be specified for Hadoop client JARs

3 participants