You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by mikewalch <gi...@git.apache.org> on 2017/03/06 21:17:14 UTC
[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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
I ran more ITs. `BadDeleteMarkersCreatedIT` and `CleanWallIT` worked for me. However, I ran all standalone ITs and several tests timed out (for reference, one was was `SessionBlockVerifyIT`) . I increased `timeout.factor` and they still timed out. I switched to the master branch and they also timed out there so it's not this PR that is causing the failures. I tried to debug the problem but I can't figure it out.
I think this PR can be merged as it does fix several bugs. If you think it makes things worse, I could do a second PR that makes minimal changes and only fixes bugs if you are willing to accept that with the tests being broken.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
> We could remove accumulo.it.cluster.standalone.server.conf as its no longer being used
Wait, I was almost with you. If this is not being used, then your PR is still broken...
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104527430
--- 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 --
s/Accumulo home directory/Accumulo installation directory/ ?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by ctubbsii <gi...@git.apache.org>.
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<Integer,String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
FYI the following does not work
```
accumulo.it.cluster.standalone.client.prefix="export ACCUMULO_CONF_DIR=/etc/accumulo-client;"
accumulo.it.cluster.standalone.server.prefix="sudo -u accumulo export ACCUMULO_CONF_DIR=/etc/accumulo-server;"
```
The quotation marks are retained which results in the being run failing (due to the quoting that the `RemoteShell` is doing). `org.apache.accumulo.test.BadDeleteMarkersCreatedIT` showed this failure to me.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
@joshelser, I will look into the test failures.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
I tested these changes by running `ReadWriteIT` against and a local cluster started by uno.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
> using a new 'client.prefix' and 'server.prefix' which replaces the the 'client.env' & 'server.env'
Maybe 'client.cmd.prefix' and 'server.cmd.prefix' instead?
> let me know if you are OK with this PR now.
I think this provides the same level of functionality we had previously, but I'll try to take another look this afternoon.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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<Integer,String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
> I also added two properties to standalone cluster testing, accumulo.it.cluster.standalone.client.env & accumulo.it.cluster.standalone.server.env, that will allow users to optionally add environment variables (in your case ACCUMULO_CONF_DIR for client and server commands.
These seems duplicative to me, analogs to accumulo.it.cluster.standalone.client.conf and accumulo.it.cluster.standalone.server.conf.
How are you envisioning those new values to be used? It seems to me that, when the a.i.c.s.client.conf and a.i.c.s.server.conf are provided, they could set those two new env variables you provided to be `ACCUMULO_CONF_DIR=<value>` for each. I _think_ that would help push things to where you want them to go.
So, if the properties provided were..
```
accumulo.it.cluster.standalone.client.conf=/etc/accumulo-client
accumulo.it.cluster.standalone.server.conf=/etc/accumulo-server
```
These would be interpreted/converted automatically `StandaloneClusterControl` to be...
```
accumulo.it.cluster.standalone.client.env="ACCUMULO_CONF_DIR=/etc/accumulo-client"
accumulo.it.cluster.standalone.server.env="ACCUMULO_CONF_DIR=/etc/accumulo-server"
```
Then, we can "deprecate" the existing properties and replace them with the new env variables. Have I interpreted this correctly?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105017625
--- 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 think we could support it for backwards compatibility in the scripts by replacing the following line:
Oh that would be wonderful. We can look into that separately (since it's not directly related to what you're doing here) if you'd like..
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
@joshelser, let me know if my changes now 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
@joshelser, I pushed another commit 12c260c0135e2c8 that should resolve your concerns. I added changes to the accumulo scripts to support `ACCUMULO_CONF_DIR` for backwards compatibility. I also added two properties to standalone cluster testing, `accumulo.it.cluster.standalone.client.env` & `accumulo.it.cluster.standalone.server.env`, that will allow users to optionally add environment variables (in your case `ACCUMULO_CONF_DIR` for client and server commands. I will do some more testing tomorrow to make sure everything works but let me know if this works 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
While I removed use of `sudo` and the 'user' property from the standalone IT properties, they can still be set using a new 'client.prefix' and 'server.prefix' which replaces the the 'client.env' & 'server.env' properties that I had before. Any values in these properties are prefixed to client & server commands. I ran about 10 ITs using standalone testing and they all passed. I also did some runs using settings similar to below to verify the new prefix properties:
```
accumulo.it.cluster.standalone.client.prefix="export ACCUMULO_CONF_DIR=/etc/accumulo-client;"
accumulo.it.cluster.standalone.server.prefix="sudo -u accumulo export ACCUMULO_CONF_DIR=/etc/accumulo-server;"
```
@joshelser, let me know if you are OK with this PR now.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
> Maybe 'client.cmd.prefix' and 'server.cmd.prefix' instead?
I updated the property names in bf9dfb4
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
> accumulo.it.cluster.standalone.client.env="ACCUMULO_CONF_DIR=/etc/accumulo-client"
accumulo.it.cluster.standalone.server.env="ACCUMULO_CONF_DIR=/etc/accumulo-server"
Yes that was how I envisioned them to be used. We could remove `accumulo.it.cluster.standalone.server.conf` as its no longer being used but `accumulo.it.cluster.standalone.client.conf` is still being used to find the location of host 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
> let me know if my changes now work for you.
Changes look OK to me now. Thanks for backing out those config changes.
Have you tested (a subset of) the ITs using a standalone cluster to make sure things still work? If you have confirmed that, +1
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
> I switched to the master branch and they also timed out there so it's not this PR that is causing the failures
That's fine -- I'm not worried about those. We're running most (all?) of these tests at $dayjob and exercising these "edge conditions" ("unsecure", kerberos auth, protected instance.secret, etc). Test failures and evolving test configuration are fine, but I couldn't stomach what would have been framework-related test failures.
+1 on your merge of these changes. Thanks for working with me.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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<Integer,String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by mikewalch <gi...@git.apache.org>.
Github user mikewalch commented on the issue:
https://github.com/apache/accumulo/pull/228
I looked at my properties file again. Actually, I don't use quotes. It looks like below:
```
accumulo.it.cluster.standalone.client.prefix=export ACCUMULO_CONF_DIR=/etc/accumulo-client;
accumulo.it.cluster.standalone.server.prefix=sudo -u accumulo export ACCUMULO_CONF_DIR=/etc/accumulo-server;
``
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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<Integer,String> 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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r104527380
--- 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 --
Change this one too if you changed the other two, please.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by ctubbsii <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
Thanks, Mike! Currently running 1.7.3 tests -- those should finish up in the hour at which point I'll run a local test of this PR.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by mikewalch <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by joshelser <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
Same for the tserver in org.apache.accumulo.test.CleanWalIT.
Locally, I'm doing:
`mvn verify -Dtest=foo -Daccumulo.it.properties=/Users/jelser/standalone.properties -Dcheckstyle.skip -Dfindbugs.skip -Drat.skip -Dfailsafe.groups=org.apache.accumulo.test.categories.StandaloneCapableClusterTests`
I'd really like to see a clean run of that before we merge this in.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo issue #228: ACCUMULO-4596 Improvements to standalone cluster testin...
Posted by joshelser <gi...@git.apache.org>.
Github user joshelser commented on the issue:
https://github.com/apache/accumulo/pull/228
Cool, thanks for clarifying, Mike.
I'm also seeing that test still failing. The GC never comes back up, fails with the below.
```
2017-03-10 16:27:05,828 [start.Main] ERROR: Uncaught exception
java.lang.NullPointerException
at org.apache.accumulo.start.Main.printCommand(Main.java:202)
at org.apache.accumulo.start.Main.printUsage(Main.java:210)
at org.apache.accumulo.start.Main.execMainClassName(Main.java:138)
at org.apache.accumulo.start.Main.main(Main.java:85)
```
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by ctubbsii <gi...@git.apache.org>.
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/228#discussion_r105258441
--- 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 --
Not really sure I understand why we're doing anything with `sudo`. Escalating privileges for testing our code seems highly questionable. I realize this already existed, and you're not trying to address it in this issue, but editing the code around it has brought it to my attention, and I'm somewhat concerned about this.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request #228: ACCUMULO-4596 Improvements to standalone cluster...
Posted by asfgit <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---