Conversation
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}/*" |
There was a problem hiding this comment.
Looking at lines starting at 58, the intention is to honor the CLASSPATH environment variable set by the user and put that first.
There was a problem hiding this comment.
This change does not circumnavigate that intention.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
- 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
- it's clear that other content in
HADOOP_HOMEare 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
left a comment
There was a problem hiding this comment.
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.
| ## Hadoop client jars location. | ||
| HADOOP_CLIENT_LIB_DIR="${HADOOP_CLIENT_LIB_DIR:-${HADOOP_HOME}/share/hadoop/client}" |
There was a problem hiding this comment.
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}/*" |
There was a problem hiding this comment.
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:
- 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
- it's clear that other content in
HADOOP_HOMEare 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.
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