[GitHub] accumulo pull request: ACCUMULO-3000: Added config option and impl...
Github user asfgit closed the pull request at: https://github.com/apache/accumulo/pull/29 --- 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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-102491443 @keith-turner @ctubbsii any thoughts on MiniAccumuloRunner changes? I think we're happy with everything else. --- 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: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-102496224 @joshelser You mean with regard to the public API? I don't see any semver violations there. It doesn't even appear to change any public API at all. It just adds an additional (optional) configuration item. Even if that were considered public API, it's an addition, so it should be fine (this is going on 1.8, right?). --- 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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-102496610 You mean with regard to the public API Yeah, that's what I was asking about. I think it was something that Keith had brought up before. I just wanted to make sure we were OK with the addition before I merge these changes in. Thanks for taking a look and confirming you're good with the change. I think it's fine. --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-102496748 Speaking of 1.8, I need to change the `@since` on the `setExistingZookeepers` to 1.8.0 from 1.7.0. --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-102499874 The `@since` is now at 1.8.0 --- 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: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30433933
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+ private static final File BASE_DIR = new
File(System.getProperty(user.dir) + /target/mini-tests/
+ + MiniAccumuloClusterExistingZooKeepersTest.class.getName());
+
+ private static final String SECRET = superSecret;
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(BASE_DIR.mkdirs() || BASE_DIR.isDirectory());
+File testDir = new File(BASE_DIR, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
--- End diff --
LGTM! Thanks for the addition!
---
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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30231160
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java
---
@@ -313,6 +321,19 @@ public MiniAccumuloConfigImpl
setZooKeeperStartupTime(long zooKeeperStartupTime)
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfigImpl setExistingZooKeepers(String
existingZooKeepers) {
+this.existingZooKeepers = existingZooKeepers;
--- End diff --
Added a sentence to the param doc that `null` will turn off the feature.
---
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: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30271336
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+ private static final File BASE_DIR = new
File(System.getProperty(user.dir) + /target/mini-tests/
+ + MiniAccumuloClusterExistingZooKeepersTest.class.getName());
+
+ private static final String SECRET = superSecret;
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(BASE_DIR.mkdirs() || BASE_DIR.isDirectory());
+File testDir = new File(BASE_DIR, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
--- End diff --
It would be good to set up something directly in this ZooKeeper that would
affect Accumulo's behavior in a way we can detect, so we can verify that it's
actually using the same instance. Maybe set the table offline by writing
directly to ZooKeeper, and checking the Accumulo API to see if the table is
online or offline? Alternatively, set a per-table custom property by writing
directly to Zookeeper, which can be checked from the API.
---
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: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on a diff in the pull request: https://github.com/apache/accumulo/pull/29#discussion_r30268106 --- Diff: .gitignore --- @@ -23,3 +23,4 @@ /.pydevproject /.idea /*.iml +*.swp --- End diff -- Ah, probably best to exclude it here, and add it to all the .gitignore files simultaneously in a separate commit. Just because it's weird to add it to one, but not the others. --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30272385
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+ private static final File BASE_DIR = new
File(System.getProperty(user.dir) + /target/mini-tests/
+ + MiniAccumuloClusterExistingZooKeepersTest.class.getName());
+
+ private static final String SECRET = superSecret;
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(BASE_DIR.mkdirs() || BASE_DIR.isDirectory());
+File testDir = new File(BASE_DIR, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
--- End diff --
Added check that the ZooKeepers string from the Connector's Instance
matches the one from the testing 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-101803898 Overall, this looks good to me. I'm particularly interested in seeing if it could be useful to use with our test harness and [revelc/zookeeper-maven-plugin](https://github.com/revelc/zookeeper-maven-plugin). --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request: https://github.com/apache/accumulo/pull/29#discussion_r30266403 --- Diff: .gitignore --- @@ -23,3 +23,4 @@ /.pydevproject /.idea /*.iml +*.swp --- End diff -- To ignore vim lock 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: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30268333
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloRunner.java
---
@@ -93,6 +94,7 @@ private static void printProperties() {
System.out.println(# + TSERVER_MEMORY_PROP + =128M);
System.out.println(# + ZOO_KEEPER_MEMORY_PROP + =128M);
System.out.println(# + JDWP_ENABLED_PROP + =false);
+System.out.println(# + EXISTING_ZOO_KEEPERS_PROP +
=localhost:2181);
--- End diff --
I realize you were probably just following what was there, but does it make
sense to print these *before* the configuration options are parsed and set?
---
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: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30269047
--- Diff: pom.xml ---
@@ -119,6 +119,8 @@
accumulo.release.version${project.version}/accumulo.release.version
!-- bouncycastle version for test dependencies --
bouncycastle.version1.50/bouncycastle.version
+!-- Curator version --
+curator.version2.7.1/curator.version
--- End diff --
Do we anticipate needing to control this on the command line, or elsewhere?
We generally don't need properties for dependency versions (especially for test
dependencies) unless the same version is used repeatedly for multiple artifacts
(like jetty or slf4j) or if we anticipate swapping it out frequently during
different build environments.
---
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: ACCUMULO-3000: Added config option and impl...
Github user ctubbsii commented on a diff in the pull request: https://github.com/apache/accumulo/pull/29#discussion_r30266262 --- Diff: .gitignore --- @@ -23,3 +23,4 @@ /.pydevproject /.idea /*.iml +*.swp --- End diff -- What's this for? --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30279997
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,91 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+ private static final File BASE_DIR = new
File(System.getProperty(user.dir) + /target/mini-tests/
+ + MiniAccumuloClusterExistingZooKeepersTest.class.getName());
+
+ private static final String SECRET = superSecret;
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(BASE_DIR.mkdirs() || BASE_DIR.isDirectory());
+File testDir = new File(BASE_DIR, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
--- End diff --
Added the ZK path check.
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30257611
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java
---
@@ -313,6 +321,19 @@ public MiniAccumuloConfigImpl
setZooKeeperStartupTime(long zooKeeperStartupTime)
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfigImpl setExistingZooKeepers(String
existingZooKeepers) {
+this.existingZooKeepers = existingZooKeepers;
--- End diff --
Having the set and clear operations in the same method is ok w/ me if the
method is only in impl and not in API.
---
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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-101419071 My use case didn't end up needing any new public APIs so, per earlier discussion, I'm going to move the code from the public API classes to the *Impl classes. --- 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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-101423939 Cool beans, thanks for the update @tanadeau. Will watch for new changes. --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-101438449 I think this PR should be good to go unless there are more comments. --- 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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request: https://github.com/apache/accumulo/pull/29#discussion_r30200397 --- Diff: minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloRunner.java --- @@ -75,6 +75,7 @@ private static final String NUM_T_SERVERS_PROP = numTServers; private static final String DIRECTORY_PROP = directory; private static final String INSTANCE_NAME_PROP = instanceName; + private static final String EXISTING_ZOO_KEEPERS_PROP = existingZooKeepers; --- End diff -- Did we already address the addition here? MiniAccumuloRunner is also public API (like MiniAccumuloCluster). Seems odd to add it here. I don't think we have a good way to hide it though (like the MiniAccumuloClusterImpl), so maybe it's ok? --- 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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r30200417
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java
---
@@ -313,6 +321,19 @@ public MiniAccumuloConfigImpl
setZooKeeperStartupTime(long zooKeeperStartupTime)
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfigImpl setExistingZooKeepers(String
existingZooKeepers) {
+this.existingZooKeepers = existingZooKeepers;
--- End diff --
Still need to address this. Javadoc on usage is probably sufficient (at
least for 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28801819
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
If its going to go into the API, I think the following needs to be
considered.
* Need a getter in the API. What should the getter do/return if not set?
* Do we want to allow the user to clear this setting, if so how does the
user clear this setting?
* Need to document in javadoc its relationship to `setZooKeeperPort()`.
Should these be made mutually exclusive? Trying to set both would cause the
2nd set to throw an exception?
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28803912
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
Could have the following (this allows setter to be more strict about its
input).
```java
public void clearExistingZookeepers()
/**
* throws IllegalArgExcep if null or empty string
*/
public void setExistingZookeepers(String ezk);
public OptionalString getExistingZookeepers();
```
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28800662
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java
---
@@ -313,6 +321,19 @@ public MiniAccumuloConfigImpl
setZooKeeperStartupTime(long zooKeeperStartupTime)
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfigImpl setExistingZooKeepers(String
existingZooKeepers) {
+this.existingZooKeepers = existingZooKeepers;
--- End diff --
Should there a be a guava precondition check for null? How does the user
unset/clear this setting?
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28803038
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
For the getter, would returing a guava Optional make sense? I have not used
Optional, I have only read about it.
---
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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-94825331 I added tests. --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28777305
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
`MiniAccumuloConfig` is leaked via a public constructor and the `getConfig`
method on `MiniAccumuloCluster`. If `MiniAccumuloConfig` doesn't have a setter
for the existing zookeepers, then it can't be used by a unit test or in general.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28789378
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
MiniAccumuloConfig is leaked via a public constructor and the getConfig
method on MiniAccumuloCluster
Yup, that's by design.
If MiniAccumuloConfig doesn't have a setter for the existing zookeepers,
then it can't be used by an integration test or in general.
Right, it would need to be used via `MiniAccumuloConfigImpl` and
`MiniAccumuloClusterImpl`. We already have many methods hidden on the impl that
are not exposed in via the non-impl variants. Nearly all of our ITs in Accumulo
use the impl variants already IIRC.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28789973
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private File baseDir = new File(System.getProperty(user.dir) +
/target/mini-tests/ + this.getClass().getName());
--- End diff --
Could be final.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-94845930 Cool, this is looking good @tanadeau. Thanks for the prompt updates! We still need to address whether or not this should go into MAC or MACImpl though. I'm still of the mindset that it shouldn't be added to the non-impl classes without good reason. I'm not convinced that this feature is something that will be directly beneficial to end users. --- 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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28791592
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
Alright, I'm not -1 on it, but I would like to get someone else to look at
these changes before they go in if that's the case.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28789572
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private File baseDir = new File(System.getProperty(user.dir) +
/target/mini-tests/ + this.getClass().getName());
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(baseDir.mkdirs() || baseDir.isDirectory());
+File testDir = new File(baseDir, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
+
+MiniAccumuloConfig config = new MiniAccumuloConfig(testDir,
superSecret);
+config.setExistingZooKeepers(zooKeeper.getConnectString());
+accumulo = new MiniAccumuloCluster(config);
+ }
+
+ @After
+ public void teardownTestCluster() {
+if (accumulo != null) {
+ try {
+accumulo.stop();
+ } catch (IOException | InterruptedException e) {
+log.warn(Failure during tear down, e);
+ }
+}
+
+if (zooKeeper != null) {
+ try {
+zooKeeper.close();
+ } catch (IOException e) {
+log.warn(Failure stopping test ZooKeeper server);
+ }
+}
+ }
+
+ @Test
+ public void multipleStartsDoesntThrowAnException() throws Exception {
--- End diff --
I don't think this test really needs to be here.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28789624
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private File baseDir = new File(System.getProperty(user.dir) +
/target/mini-tests/ + this.getClass().getName());
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(baseDir.mkdirs() || baseDir.isDirectory());
+File testDir = new File(baseDir, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
+
+MiniAccumuloConfig config = new MiniAccumuloConfig(testDir,
superSecret);
+config.setExistingZooKeepers(zooKeeper.getConnectString());
+accumulo = new MiniAccumuloCluster(config);
+ }
+
+ @After
+ public void teardownTestCluster() {
+if (accumulo != null) {
+ try {
+accumulo.stop();
+ } catch (IOException | InterruptedException e) {
+log.warn(Failure during tear down, e);
+ }
+}
+
+if (zooKeeper != null) {
+ try {
+zooKeeper.close();
+ } catch (IOException e) {
+log.warn(Failure stopping test ZooKeeper server);
+ }
+}
+ }
+
+ @Test
+ public void multipleStartsDoesntThrowAnException() throws Exception {
+// In 1.6.0, multiple start's did not throw an exception as advertised
+accumulo.start();
+try {
+ accumulo.start();
+} finally {
+ accumulo.stop();
+}
+ }
+
+ @Test
+ public void multipleStopsIsAllowed() throws Exception {
--- End diff --
Rename this test to what it's actually testing 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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] accumulo pull request: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28789662
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private File baseDir = new File(System.getProperty(user.dir) +
/target/mini-tests/ + this.getClass().getName());
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(baseDir.mkdirs() || baseDir.isDirectory());
+File testDir = new File(baseDir, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
+
+MiniAccumuloConfig config = new MiniAccumuloConfig(testDir,
superSecret);
+config.setExistingZooKeepers(zooKeeper.getConnectString());
+accumulo = new MiniAccumuloCluster(config);
+ }
+
+ @After
+ public void teardownTestCluster() {
+if (accumulo != null) {
+ try {
+accumulo.stop();
+ } catch (IOException | InterruptedException e) {
+log.warn(Failure during tear down, e);
+ }
+}
+
+if (zooKeeper != null) {
+ try {
+zooKeeper.close();
+ } catch (IOException e) {
+log.warn(Failure stopping test ZooKeeper server);
+ }
+}
+ }
+
+ @Test
+ public void multipleStartsDoesntThrowAnException() throws Exception {
+// In 1.6.0, multiple start's did not throw an exception as advertised
+accumulo.start();
+try {
+ accumulo.start();
+} finally {
+ accumulo.stop();
+}
+ }
+
+ @Test
+ public void multipleStopsIsAllowed() throws Exception {
+accumulo.start();
+
+Connector conn = accumulo.getConnector(root, superSecret);
+conn.tableOperations().create(foo);
+assertTrue(conn.tableOperations().list().contains(foo));
+
+accumulo.stop();
+accumulo.stop();
--- End diff --
Remove the second stop.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28789943
--- Diff:
minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterExistingZooKeepersTest.java
---
@@ -0,0 +1,100 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the License); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an AS IS BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.accumulo.minicluster;
+
+import static org.junit.Assert.assertTrue;
+
+import java.io.File;
+import java.io.IOException;
+
+import org.apache.accumulo.core.client.Connector;
+import org.apache.commons.io.FileUtils;
+import org.apache.curator.test.TestingServer;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Rule;
+import org.junit.Test;
+import org.junit.rules.TestName;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class MiniAccumuloClusterExistingZooKeepersTest {
+
+ private static final Logger log =
LoggerFactory.getLogger(MiniAccumuloClusterExistingZooKeepersTest.class);
+ private File baseDir = new File(System.getProperty(user.dir) +
/target/mini-tests/ + this.getClass().getName());
+ private TestingServer zooKeeper;
+ private MiniAccumuloCluster accumulo;
+
+ @Rule
+ public TestName testName = new TestName();
+
+ @Before
+ public void setupTestCluster() throws Exception {
+assertTrue(baseDir.mkdirs() || baseDir.isDirectory());
+File testDir = new File(baseDir, testName.getMethodName());
+FileUtils.deleteQuietly(testDir);
+assertTrue(testDir.mkdir());
+
+zooKeeper = new TestingServer();
+
+MiniAccumuloConfig config = new MiniAccumuloConfig(testDir,
superSecret);
--- End diff --
Would be nice to make `superSecret` a static final constant
---
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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-94843575 Addressed test comments. --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28790916
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
I want to use this outside of internal Accumulo tests. I *really* don't
want to depend on internal impl classes to do so.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28818441
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
passing null to setEZK is unexpected
I'm used to seeing a note on Javadoc which states when a setter is
overloaded. Personally, that's sufficient.
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-94870883 I'm not convinced that this feature is something that will be directly beneficial to end users. It would not be beneficial for [Fluo](https://github.com/fluo-io/fluo). Fluo uses the accumulo maven plugin to start one mini instance that all of its ITs use. --- 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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-94579191 Overall, this looks nice. Thanks! Could you possibly add an integration test that verifies that you can use the new methods to start up a MiniAccumuloCluster instance? You could lift what MiniAccumuloClusterImpl already does to start ZooKeeper and then just do something simple like read from the Accumulo metadata table as a can I actually connect to and interact with 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on the pull request: https://github.com/apache/accumulo/pull/29#issuecomment-94579312 That makes sense. --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28733796
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
Okay. If you're good with the runner using `MiniAccumuloConfigImpl`, then I
can make that change.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28733986
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
I think being conservative here makes the most sense. It is much easier to
promote things to public API later on if they are super-useful than it is to
remove things from public API (as that takes at least a year at our current
release cadence). Thanks for making the change.
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28734323
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
Mini runner is user facing. If we add it there, its very similar to adding
it to the API from a maintenance perspective.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28732817
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
MiniAccumuloConfig is in the public API. I'm not sure we want to make this
change so outwardly facing. It might make more sense to only expose it on
MiniAccumuloConfigImpl since the primary use case would be for internal
Accumulo testing. Thoughts?
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28733538
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
The runner doesn't have to use MiniAccumuloConfig, it could be using the
MiniAccumuloConfigImpl which would get around that concern. For more context,
we are very hesitant about putting new things into our public API. Every
method/class in the public API increases our API compatibility debt. If it's
not needed for user functionality, we tend to err on the side of not including
things to make maintaining our compatibility easier.
---
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: ACCUMULO-3000: Added config option and impl...
GitHub user tanadeau opened a pull request: https://github.com/apache/accumulo/pull/29 ACCUMULO-3000: Added config option and impl to use existing ZooKeeper This creates a new MAC config option `existingZooKeepers` that can be optionally set to a ZK connection string (e.g., localhost:2181). If this is set, the MAC will not spin up a ZK process nor create a ZK config directory. Since the same ZK instance can now be used repeatedly, any existing Accumulo instances with the same instance name are removed. You can merge this pull request into a Git repository by running: $ git pull https://github.com/tanadeau/accumulo mac-with-existing-zookeeper Alternatively you can review and apply these changes as the patch at: https://github.com/apache/accumulo/pull/29.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 #29 commit bcdd8c37d8a3037cf624354ccee295fbb5edcd5f Author: Trent Nadeau tnad...@42six.com Date: 2015-04-16T14:35:54Z ACCUMULO-3000: Added config option and impl to use existing ZooKeeper --- 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: ACCUMULO-3000: Added config option and impl...
Github user tanadeau commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28733135
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
I have a use case of running the MAC with an existing ZK from the command
line. This means that I need to use the runner class that uses
`MiniAccumuloConfig`.
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28750135
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
It is, but I don't see a place where we actually leak
MiniAccumuloCluster/Config through that class. Did I miss something?
No, not implying anything leaks.
If we want to be really picky (and these changes go on the impl), we
should not even expose the new property via MiniAccumuloRunner. Is this what
you were pointing to?
Just thinking that if we add it to mini runner, that we will need to
continue to support that option in mini runner. I think its very similar to
adding to API. I am not opposed or in favor of adding an option to mini
runner or API. I don't know enough about the performance impact to have an
opinion about adding to API or mini runner. I am really curious about the
performance impact.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28747812
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
Mini runner is user facing
It is, but I don't see a place where we actually leak
MiniAccumuloCluster/Config through that class. Did I miss something? If we want
to be really picky (and these changes go on the impl), we should not even
expose the new property via MiniAccumuloRunner. Is this what you were pointing
to?
---
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: ACCUMULO-3000: Added config option and impl...
Github user keith-turner commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28750186
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
Also, I am not opposed to adding an option to just mini runner.
---
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: ACCUMULO-3000: Added config option and impl...
Github user joshelser commented on a diff in the pull request:
https://github.com/apache/accumulo/pull/29#discussion_r28750299
--- Diff:
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
---
@@ -108,6 +108,19 @@ public MiniAccumuloConfig setZooKeeperStartupTime(long
zooKeeperStartupTime) {
}
/**
+ * Configure an existing ZooKeeper instance to use. Calling this method
is optional. If not set, a new ZooKeeper instance is created.
+ *
+ * @param existingZooKeepers
+ * Connection string for a already-running ZooKeeper instance
+ *
+ * @since 1.7.0
+ */
+ public MiniAccumuloConfig setExistingZooKeepers(String
existingZooKeepers) {
--- End diff --
I am really curious about the performance impact.
Looking at some [Jenkins
logs](https://secure.penguinsinabox.com/jenkins/job/Accumulo-Master-Integration-Tests/ws/test/target/mini-tests/org.apache.accumulo.test.ConditionalWriterIT_testBasic/logs/),
it looks like starting ZK and waiting for it to come up takes ~3seconds (time
on first ZK log msg compared to time on first tserver log msg). More
quantitative measurements would be cool to collect as well.
---
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.
---
