[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user asfgit closed the pull request at: https://github.com/apache/accumulo/pull/228 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105457380
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +147,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, serverEnv,
accumuloPath, Admin.class.getName(), "stopAll"};
--- End diff --
I wasn't assuming anything. I was observing my own ignorance about the
reasons why this was being done. In any case, you answered the question I
should have asked, instead ("Why is this being done?").
Your response indicates that the reason this is being done is so that this
test framework can switch users to control an external running Accumulo
instance, running as a specific user, assuming `sudo` is configured in a very
particular way to allow running the necessary executables without a tty or
authentication.
I now understand why it uses `sudo`, but its inclusion suggests that this
code exists not to test the upstream Accumulo project, but to test specific
downstream products. That makes me a bit uncomfortable because I don't like the
idea of the upstream project being burdened with the maintenance of tests for
downstream vendors. However, I can also see it the other way around: that the
upstream project has an interest in providing some test code to ensure any
downstream packaging meets some minimum standards of quality so that Accumulo.
So, I'm not necessarily going to argue we should remove it.
I would, however, argue that we "genericize" it a bit, so we don't have to
directly support the use of privilege escalation features of an operating
system for our tests. We can support that indirectly by allowing the user to
supply the launch command. The direct use of `sudo` is mostly what makes me
uncomfortable. I've seen a lot of open source projects code, and this is the
first time I've ever seen that used in any project's test suite that wasn't
`sudo` itself.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105443724
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +147,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, serverEnv,
accumuloPath, Admin.class.getName(), "stopAll"};
--- End diff --
You're incorrectly assuming that the tests are always running as the user
that is running Accumulo.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105409269
--- Diff: assemble/bin/accumulo ---
@@ -39,7 +39,7 @@ function main() {
# Set up variables needed by accumulo-env.sh
export bin="$( cd -P "$( dirname "${SOURCE}" )" && pwd )"
export basedir=$( cd -P "${bin}"/.. && pwd )
- export conf="${basedir}/conf"
+ export conf="${ACCUMULO_CONF_DIR-${basedir}/conf}"
--- End diff --
Nice catch. It's supposed to be `:-`. I will fix.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105288227
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +147,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, serverEnv,
accumuloPath, Admin.class.getName(), "stopAll"};
--- End diff --
I'm not sure what needs to be worked around. It's not clear to me why we
would ever need to do that.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105259450
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +147,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, serverEnv,
accumuloPath, Admin.class.getName(), "stopAll"};
--- End diff --
Feel free to break this out. I'm not sure how else you think this could be
worked around..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user ctubbsii commented on a diff in the pull request: https://github.com/apache/accumulo/pull/228#discussion_r105247045 --- Diff: TESTING.md --- @@ -142,11 +145,8 @@ what is executed for a standalone cluster. `mvn verify -Daccumulo.it.properties=/home/user/my_cluster.properties` For the optional properties, each of them will be extracted from the environment if not explicitly provided. -Specifically, `ACCUMULO_HOME` and `ACCUMULO_CONF_DIR` are used to ensure the correct version of the bundled -Accumulo scripts are invoked and, in the event that multiple Accumulo processes exist on the same physical machine, -but for different instances, the correct version is terminated. `HADOOP_CONF_DIR` is used to ensure that the necessary -files to construct the FileSystem object for the cluster can be constructed (e.g. core-site.xml and hdfs-site.xml), -which is typically required to interact with HDFS. +`HADOOP_CONF_DIR` is used to ensure that the necessary files to construct the FileSystem object for the cluster can --- End diff -- Could simplify this to just be "additional classpath items". --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105247221
--- Diff: assemble/bin/accumulo ---
@@ -39,7 +39,7 @@ function main() {
# Set up variables needed by accumulo-env.sh
export bin="$( cd -P "$( dirname "${SOURCE}" )" && pwd )"
export basedir=$( cd -P "${bin}"/.. && pwd )
- export conf="${basedir}/conf"
+ export conf="${ACCUMULO_CONF_DIR-${basedir}/conf}"
--- End diff --
Is this supposed to be `:-` or just `-`? (same question in the repeated
locations below)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105253899
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -123,11 +120,12 @@ public int exec(Class clz, String[] args) throws
IOException {
public Entry execMapreduceWithStdout(Class clz,
String[] args) throws IOException {
String host = "localhost";
-String[] cmd = new String[3 + args.length];
-cmd[0] = getToolPath();
-cmd[1] = getJarFromClass(clz);
-cmd[2] = clz.getName();
-for (int i = 0, j = 3; i < args.length; i++, j++) {
+String[] cmd = new String[4 + args.length];
+cmd[0] = getAccumuloUtilPath();
+cmd[1] = "hadoop-jar";
+cmd[2] = getJarFromClass(clz);
+cmd[3] = clz.getName();
+for (int i = 0, j = 4; i < args.length; i++, j++) {
--- End diff --
This whole thing would be easier to read if it used ArrayList for cmd
instead, and just appended to the end.
At the very least, we could protect against mistakes here, by setting
`offset = 4` and using that to construct the array size (`new String[offset +
args.length]`) and in this loop:
```java
for (int i = offset; i < args.length; i++) {
cmd[i + offset] = "'" + args[i] + "'";
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105017334
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
While all Java code and Accumulo documentation should remove use of
`ACCUMULO_CONF_DIR`, I think we could support it for backwards compatibility in
the scripts by replacing the following line:
```bash
export conf="${basedir}/conf"`
```
in the `accumulo` & `accumulo-service` scripts with the following:
```bash
export conf="${ACCUMULO_CONF_DIR-${basedir}/conf}"
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105013083
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
Functionally, yes, I think that would work.
> However, I don't think it's hard at this point to add a solution for
separate client & server configuration directories that avoid use of
ACCUMULO_CONF_DIR.
Yeah, it doesn't matter too much to me the means, just that the
functionality exists.
> ${conf} defaults to the conf directory in the Accumulo tarball
installation
Ok, and that's extrapolated based on the assumption that the `bin/` dir is
next to the `conf/` dir (or at least a symlink exists to enable that). Sounds
fine.
As a general statement, I'm lamenting that task that this change will
entail to update all of my downstream docs/code to make this work. This will be
a very cross cutting change for me to have to make and will be a pain in the
butt. Sadly, we don't have a good strategy for defining what compatibility is
here (and what the 1.x to 2.x means).
FYI @busbey as this may also affect things on the CM side..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105010895
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
I see your point that a solution hasn't been presented yet. I can add
ACCUMULO_CONF_DIR back to this PR until there is one. However, I don't think
it's hard at this point to add a solution for separate client & server
configuration directories that avoid use of ACCUMULO_CONF_DIR.
After #223 is merged, the accumulo command will set up the java `CLASSPATH`
using bash code below:
```bash
CLASSPATH="${conf}:${lib}/*:${CLASSPATH}"
```
`${conf}` defaults to the `conf` directory in the Accumulo tarball
installation but this can be overridden by downstream packaging to anything
(i.e `/etc/accumulo`). This directory contains the server config.
We could also add some default client configuration directories after the
server configuration directory. Something like below:
```bash
CLASSPATH="${conf}:${conf}/client:${HOME}/.accumulo:${lib}/*:${CLASSPATH}"
```
Any accumulo command would try to pull configuration from $conf before
falling back to $conf/client, and then $HOME/.accumulo.
If you are packaging downstream, you can even replace this line with other
locations:
```bash
CLASSPATH="${conf}:/etc/accumulo-client:${HOME}/.accumulo:${lib}/*:${CLASSPATH}"
```
Does this work for you?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104998805
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
> We should be getting away from using ACCUMULO_CONF_DIR as it setting
these env variables is very prone to errors.
That's fine if you want to move away from them, but I don't see a solution
presented as to how you don't break existing functionality :). As long as we
have to keep the tracer user's password and instance.secret in
accumulo-site.xml, we're stuck having a copy of those files which are not
globally readable.
Pushing harder on use of the client.conf is one possibility but this is in
a half-cocked state, IMO.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104998228
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -170,9 +162,8 @@ public void adminStopAll() throws IOException {
public void setGoalState(String goalState) throws IOException {
requireNonNull(goalState, "Goal state must not be null");
checkArgument(MasterGoalState.valueOf(goalState) != null, "Unknown
goal state: " + goalState);
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, SetGoalState.class.getName(), goalState};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
SetGoalState.class.getName(), goalState};
--- End diff --
Again, same thing as the above with the `stopAll`. Pretty sure this
requires the correct `instance.secret`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104997993
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
> So you are not OK with the removal of ACCUMULO_CONF_DIR +
serverAccumuloConfDir
Oh, no, I'm not because you're still breaking things. I didn't realize you
didn't revert this change.
> Why do you need this?
See my previous comment: "StopAll is another utility which requires the
correct instance.secret.". It's the same reason I -1'ed the removal of the
accumuloServerConf property in the first place..
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104996774
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
So you are not OK with the removal of `ACCUMULO_CONF_DIR +
serverAccumuloConfDir`? Why do you need this? I am assuming the Accumulo
installation has all server config in the default configuration directory. We
should be getting away from using `ACCUMULO_CONF_DIR` as it setting these env
variables is very prone to errors.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104992750
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -123,11 +114,12 @@ public int exec(Class clz, String[] args) throws
IOException {
public Entry execMapreduceWithStdout(Class clz,
String[] args) throws IOException {
String host = "localhost";
-String[] cmd = new String[3 + args.length];
-cmd[0] = getToolPath();
-cmd[1] = getJarFromClass(clz);
-cmd[2] = clz.getName();
-for (int i = 0, j = 3; i < args.length; i++, j++) {
+String[] cmd = new String[4 + args.length];
+cmd[0] = getAccumuloUtilPath();
+cmd[1] = "hadoop-jar";
+cmd[2] = getJarFromClass(clz);
+cmd[3] = clz.getName();
--- End diff --
Okie doke. Half of this was me wondering how it fails when the
configuration is actually wrong (e.g. we can't find the hadoop
installation/jars), but that isn't related to this changeset.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104992602
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
Same issue as above with dropping the server conf (so we can probably
ignore it). `StopAll` is another utility which requires the correct
`instance.secret`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request: https://github.com/apache/accumulo/pull/228#discussion_r104678751 --- Diff: TESTING.md --- @@ -117,8 +117,8 @@ The following properties can be used to configure a standalone cluster: - `accumulo.it.cluster.standalone.zookeepers`, Required: ZooKeeper quorum used by the standalone cluster - `accumulo.it.cluster.standalone.instance.name`, Required: Accumulo instance name for the cluster - `accumulo.it.cluster.standalone.hadoop.conf`, Required: `HADOOP_CONF_DIR` -- `accumulo.it.cluster.standalone.home`, Optional: `ACCUMULO_HOME` -- `accumulo.it.cluster.standalone.conf`, Optional: `ACCUMULO_CONF_DIR` +- `accumulo.it.cluster.standalone.home`, Required: Accumulo home directory on cluster --- End diff -- Fixed --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request: https://github.com/apache/accumulo/pull/228#discussion_r104678703 --- Diff: TESTING.md --- @@ -117,8 +117,8 @@ The following properties can be used to configure a standalone cluster: - `accumulo.it.cluster.standalone.zookeepers`, Required: ZooKeeper quorum used by the standalone cluster - `accumulo.it.cluster.standalone.instance.name`, Required: Accumulo instance name for the cluster - `accumulo.it.cluster.standalone.hadoop.conf`, Required: `HADOOP_CONF_DIR` --- End diff -- Fixed --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104678440
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -123,11 +114,12 @@ public int exec(Class clz, String[] args) throws
IOException {
public Entry execMapreduceWithStdout(Class clz,
String[] args) throws IOException {
String host = "localhost";
-String[] cmd = new String[3 + args.length];
-cmd[0] = getToolPath();
-cmd[1] = getJarFromClass(clz);
-cmd[2] = clz.getName();
-for (int i = 0, j = 3; i < args.length; i++, j++) {
+String[] cmd = new String[4 + args.length];
+cmd[0] = getAccumuloUtilPath();
+cmd[1] = "hadoop-jar";
+cmd[2] = getJarFromClass(clz);
+cmd[3] = clz.getName();
--- End diff --
It fails just like the previous `tool.sh` command as `accumulo-util
hadoop-jar` is the same underlying logic.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104537251
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
Not sure about your comment. It looks like it was previously set for user
to read Accumulo services' configuration files..
```java
String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user mikewalch commented on a diff in the pull request: https://github.com/apache/accumulo/pull/228#discussion_r104535648 --- Diff: minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneAccumuloCluster.java --- @@ -57,7 +57,7 @@ private Instance instance; private ClientConfiguration clientConf; - private String accumuloHome, clientAccumuloConfDir, serverAccumuloConfDir, hadoopConfDir; --- End diff -- OK, I will bring back client and server config directories. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/accumulo/pull/228#discussion_r104527998 --- Diff: minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneAccumuloCluster.java --- @@ -57,7 +57,7 @@ private Instance instance; private ClientConfiguration clientConf; - private String accumuloHome, clientAccumuloConfDir, serverAccumuloConfDir, hadoopConfDir; --- End diff -- There is a reason that both of these exist. Some tests do things that require knowledge of the `instance.secret` (generally, configuration values which are sensitive). Good deployment practices dictate that there is a separation here -- as such, I don't see how this simplification is helpful. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104528620
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -149,9 +141,8 @@ String getJarFromClass(Class clz) {
@Override
public void adminStopAll() throws IOException {
-File confDir = getConfDir();
-String master = getHosts(new File(confDir, "masters")).get(0);
-String[] cmd = new String[] {SUDO_CMD, "-u", user, ACCUMULO_CONF_DIR +
serverAccumuloConfDir, accumuloPath, Admin.class.getName(), "stopAll"};
+String master = getHosts(MASTER_HOSTS_FILE).get(0);
+String[] cmd = new String[] {SUDO_CMD, "-u", user, accumuloPath,
Admin.class.getName(), "stopAll"};
--- End diff --
Ah, this is also a good example of what would be broken by your change.
We should not be requiring that a user be privileged to read the Accumulo
services' configuration files.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104528417
--- Diff:
minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java
---
@@ -123,11 +114,12 @@ public int exec(Class clz, String[] args) throws
IOException {
public Entry execMapreduceWithStdout(Class clz,
String[] args) throws IOException {
String host = "localhost";
-String[] cmd = new String[3 + args.length];
-cmd[0] = getToolPath();
-cmd[1] = getJarFromClass(clz);
-cmd[2] = clz.getName();
-for (int i = 0, j = 3; i < args.length; i++, j++) {
+String[] cmd = new String[4 + args.length];
+cmd[0] = getAccumuloUtilPath();
+cmd[1] = "hadoop-jar";
+cmd[2] = getJarFromClass(clz);
+cmd[3] = clz.getName();
--- End diff --
This is nice. How do this fail in the case of mis-configuration?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
GitHub user mikewalch opened a pull request: https://github.com/apache/accumulo/pull/228 ACCUMULO-4596 Improvements to standalone cluster testing * Limited use of bash env variables in standalone cluster testing * Fixed standalone cluster testing sow that it work with Accumulo 2.0 script changes * Users now only need to configure Accumulo conf directory on client You can merge this pull request into a Git repository by running: $ git pull https://github.com/mikewalch/accumulo standalone Alternatively you can review and apply these changes as the patch at: https://github.com/apache/accumulo/pull/228.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #228 --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
