----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28214/#review62156 -----------------------------------------------------------
minicluster/src/main/java/org/apache/accumulo/cluster/ClusterControl.java <https://reviews.apache.org/r/28214/#comment104114> Not sure if it's better to accept `String[] args` or `String... args`. MiniAccumuloClusterImpl's `exec` method did accept the varargs variant. minicluster/src/main/java/org/apache/accumulo/cluster/ClusterControl.java <https://reviews.apache.org/r/28214/#comment104116> Are the variants that accept a hostname even worthwhile? The caller would have to know where things are running which is hard to get at presently. minicluster/src/main/java/org/apache/accumulo/cluster/RemoteShell.java <https://reviews.apache.org/r/28214/#comment104115> I think this would be sufficient to allow SSH to work in most environments. You can provide a specific key (any options) and the user to ssh as. Not sure if I'm missing anything else? minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java <https://reviews.apache.org/r/28214/#comment104117> This is rather brittle because it expects the tests to be running on a properly configured host for the instance that you're connecting to. Acceptable for a first pass? minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java <https://reviews.apache.org/r/28214/#comment104118> Oops. minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java <https://reviews.apache.org/r/28214/#comment104119> StandaloneAccumuloCluster obviates the need for the existingInstance support inside MAC. Don't need to kill it in the initial code, though. minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterStartStopTest.java <https://reviews.apache.org/r/28214/#comment104120> Was an incorrectly named method. test/src/main/java/org/apache/accumulo/test/TestMultiTableIngest.java <https://reviews.apache.org/r/28214/#comment104121> Let's us pass down a specific prefix for the tables that are created which allow us to automatically clean up those tables in Before/After methods. test/src/test/java/org/apache/accumulo/harness/AccumuloClusterIT.java <https://reviews.apache.org/r/28214/#comment104122> This is really gross and not properly implemented. I think to make this correctly work, we'd have to get at core-site.xml and/or hdfs-site.xml. We might be able to guess at this from Accumulo configuration instead of requiring it to be explicitly passed in. Without the ability to get the filesystem for the cluster, we can't port over tests that exercise functionality like bulk import or import/export table. test/src/test/java/org/apache/accumulo/harness/AccumuloClusterIT.java <https://reviews.apache.org/r/28214/#comment104123> I wasn't really sure what should be done here. It helps keep the code more backwards compatible but I don't like adding implementation specifics to this class. It might be possible to push this down to the AccumuloCluster implementation. The worry for a standalone instance is that we'd want it returned to it's original state after the test. To do that, we'd also have to add a teardown hook to the AccumuloCluster implementation that this class could invoke. test/src/test/java/org/apache/accumulo/harness/AccumuloClusterIT.java <https://reviews.apache.org/r/28214/#comment104124> Because the Before methods of the parent class are run before the children class, even if a test did not want to run against a MiniCluster, the MiniCluster would still be started before it could make any JUnit Assume call. Providing a method that the implementation can override ensures that we don't waste time starting some cluster that we don't want to use. test/src/test/java/org/apache/accumulo/harness/MiniClusterHarness.java <https://reviews.apache.org/r/28214/#comment104125> Whitespace test/src/test/java/org/apache/accumulo/harness/conf/AccumuloStandaloneClusterConfiguration.java <https://reviews.apache.org/r/28214/#comment104126> Will need to add some more pieces here to run against an instance with SSL enabled. test/src/test/java/org/apache/accumulo/test/Accumulo3047IT.java <https://reviews.apache.org/r/28214/#comment104127> This turns out to be a really common pattern that cropped up across the test code. Providing some mechanism to automatically do this in AccumuloClusterIT would be nice. test/src/test/java/org/apache/accumulo/test/MetaSplitIT.java <https://reviews.apache.org/r/28214/#comment104128> Maybe this one wouldn't be so bad to run against a standalone cluster? I think I initially shyed away from it because of all the metadata and root table mucking. test/src/test/java/org/apache/accumulo/test/functional/AbstractMacIT.java <https://reviews.apache.org/r/28214/#comment104129> I think AbstractMacIT should just inherit these methods from AccumuloIT for consistency. test/src/test/java/org/apache/accumulo/test/functional/BulkFileIT.java <https://reviews.apache.org/r/28214/#comment104134> These changes need to be reverted. Had tried to port this one and ultimately gave up. test/src/test/java/org/apache/accumulo/test/functional/RestartIT.java <https://reviews.apache.org/r/28214/#comment104135> This is a big downside of implementing ClusterControl how I did. It's easy to just run something and get the exit code, but it's difficult to spawn a background process and wait for it to return. Is it worth it to provide both of these methods? test/src/test/java/org/apache/accumulo/test/functional/RestartIT.java <https://reviews.apache.org/r/28214/#comment104136> Providing methods that accept a ServerType and check/wait for the ZooLocks in AccumuloClusterIT might be generally useful. test/src/test/java/org/apache/accumulo/test/functional/SimpleMacIT.java <https://reviews.apache.org/r/28214/#comment104137> Outdated comment. self-review... - Josh Elser On Nov. 19, 2014, 4:02 p.m., Josh Elser wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28214/ > ----------------------------------------------------------- > > (Updated Nov. 19, 2014, 4:02 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-3167 > https://issues.apache.org/jira/browse/ACCUMULO-3167 > > > Repository: accumulo > > > Description > ------- > > We have a large number of good tests which are only capable of being run > against a MiniAccumuloCluster. This is undesirable because it limits our > tests to the scope of what MiniAccumuloCluster provides, or implements as a > standalone cluster does. An accurate test environment would be a true > deployment with distributed HDFS and ZooKeper instances that back a > distributed Accumulo instance. This patch introduces a > StandaloneAccumuloCluster, in addition to a few other new interfaces > (ClusterControl) which help support the necessary functionality. The > StandaloneAccumuloCluster is the "MiniAccumuloCluster" counterpart to a > distributed cluster. > > Given the StandaloneAccumuloCluster, many of the integration tests need to be > rewritten in such a way that support both a MiniAccumuloCluster and the > Standalone cluster. While being a painful set of changes, this does help > generalize some of the tests and conform them to some best practices to > simplify things. > > I also nuked some initial interfaces which I initially stubbed out because > they turned out not being useful. > > > Diffs > ----- > > minicluster/src/main/java/org/apache/accumulo/cluster/AccumuloCluster.java > c982de0 > minicluster/src/main/java/org/apache/accumulo/cluster/AccumuloClusters.java > 50cb9db > minicluster/src/main/java/org/apache/accumulo/cluster/AccumuloConfig.java > 0df2348 > minicluster/src/main/java/org/apache/accumulo/cluster/ClusterControl.java > PRE-CREATION > minicluster/src/main/java/org/apache/accumulo/cluster/RemoteShell.java > PRE-CREATION > > minicluster/src/main/java/org/apache/accumulo/cluster/RemoteShellOptions.java > PRE-CREATION > minicluster/src/main/java/org/apache/accumulo/cluster/package-info.java > f1b649d > > minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneAccumuloCluster.java > PRE-CREATION > > minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java > PRE-CREATION > > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloCluster.java > 3e8c5a0 > minicluster/src/main/java/org/apache/accumulo/minicluster/ServerType.java > 3590a20 > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterControl.java > PRE-CREATION > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java > 7283c19 > > minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java > 2d7103e > > minicluster/src/test/java/org/apache/accumulo/cluster/AccumuloClustersTest.java > e368240 > > minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterStartStopTest.java > 2031b11 > > minicluster/src/test/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImplTest.java > b19d289 > test/src/main/java/org/apache/accumulo/test/TestMultiTableIngest.java > 16f0b3f > test/src/test/java/org/apache/accumulo/harness/AccumuloClusterIT.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/harness/AccumuloIT.java PRE-CREATION > > test/src/test/java/org/apache/accumulo/harness/MiniClusterConfigurationCallback.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/harness/MiniClusterHarness.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/harness/SharedMiniClusterIT.java > PRE-CREATION > > test/src/test/java/org/apache/accumulo/harness/conf/AccumuloClusterConfiguration.java > PRE-CREATION > > test/src/test/java/org/apache/accumulo/harness/conf/AccumuloClusterPropertyConfiguration.java > PRE-CREATION > > test/src/test/java/org/apache/accumulo/harness/conf/AccumuloMiniClusterConfiguration.java > PRE-CREATION > > test/src/test/java/org/apache/accumulo/harness/conf/AccumuloStandaloneClusterConfiguration.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/Accumulo3010IT.java 791b1d5 > test/src/test/java/org/apache/accumulo/test/Accumulo3030IT.java 3512e4a > test/src/test/java/org/apache/accumulo/test/Accumulo3047IT.java 70e1c30 > test/src/test/java/org/apache/accumulo/test/AuditMessageIT.java 49b5d70 > test/src/test/java/org/apache/accumulo/test/BatchWriterIT.java ca72e7a > test/src/test/java/org/apache/accumulo/test/CleanWalIT.java d0bfe3f > test/src/test/java/org/apache/accumulo/test/ConditionalWriterIT.java > 516cd46 > > test/src/test/java/org/apache/accumulo/test/ConfigurableMajorCompactionIT.java > 899b41b > test/src/test/java/org/apache/accumulo/test/DeleteRowsIT.java ff67e89 > test/src/test/java/org/apache/accumulo/test/ExistingMacIT.java ec72281 > test/src/test/java/org/apache/accumulo/test/ImportExportIT.java a48ed9d > test/src/test/java/org/apache/accumulo/test/InterruptibleScannersIT.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/KeyValueEqualityTest.java > 1302b23 > test/src/test/java/org/apache/accumulo/test/MetaConstraintRetryIT.java > b3c3640 > test/src/test/java/org/apache/accumulo/test/MetaSplitIT.java 50a1446 > test/src/test/java/org/apache/accumulo/test/MultiTableBatchWriterIT.java > 484c048 > test/src/test/java/org/apache/accumulo/test/NamespacesIT.java 8188deb > test/src/test/java/org/apache/accumulo/test/NoMutationRecoveryIT.java > 87ad1a3 > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 4457e70 > test/src/test/java/org/apache/accumulo/test/SplitRecoveryIT.java 96d3a1a > test/src/test/java/org/apache/accumulo/test/TableConfigurationUpdateIT.java > 0d9a211 > test/src/test/java/org/apache/accumulo/test/TableOperationsIT.java bb12279 > test/src/test/java/org/apache/accumulo/test/VolumeIT.java d5c940d > test/src/test/java/org/apache/accumulo/test/functional/AbstractMacIT.java > 22e46ff > > test/src/test/java/org/apache/accumulo/test/functional/AccumuloInputFormatIT.java > ad84960 > test/src/test/java/org/apache/accumulo/test/functional/AddSplitIT.java > 05de342 > > test/src/test/java/org/apache/accumulo/test/functional/BadIteratorMincIT.java > 9c4492e > > test/src/test/java/org/apache/accumulo/test/functional/BalanceInPresenceOfOfflineTableIT.java > 887aee4 > > test/src/test/java/org/apache/accumulo/test/functional/BatchScanSplitIT.java > 688a326 > > test/src/test/java/org/apache/accumulo/test/functional/BatchWriterFlushIT.java > 465936e > test/src/test/java/org/apache/accumulo/test/functional/BigRootTabletIT.java > b021c3a > test/src/test/java/org/apache/accumulo/test/functional/BinaryIT.java > 9c0edaa > test/src/test/java/org/apache/accumulo/test/functional/BinaryStressIT.java > a60c2d5 > test/src/test/java/org/apache/accumulo/test/functional/BloomFilterIT.java > 8f6b830 > test/src/test/java/org/apache/accumulo/test/functional/BulkFileIT.java > a7cf6bd > test/src/test/java/org/apache/accumulo/test/functional/BulkIT.java 831dcd4 > > test/src/test/java/org/apache/accumulo/test/functional/ChaoticBalancerIT.java > 8afb3d2 > test/src/test/java/org/apache/accumulo/test/functional/ClassLoaderIT.java > d71819e > test/src/test/java/org/apache/accumulo/test/functional/CleanUpIT.java > 79bbb90 > test/src/test/java/org/apache/accumulo/test/functional/CloneTestIT.java > 505dd5a > test/src/test/java/org/apache/accumulo/test/functional/CombinerIT.java > 69f9134 > test/src/test/java/org/apache/accumulo/test/functional/ConcurrencyIT.java > 92bd714 > > test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java > 9185e1b > test/src/test/java/org/apache/accumulo/test/functional/ConstraintIT.java > 7e5944e > test/src/test/java/org/apache/accumulo/test/functional/CreateAndUseIT.java > 5b5249b > > test/src/test/java/org/apache/accumulo/test/functional/CreateManyScannersIT.java > aed38e8 > test/src/test/java/org/apache/accumulo/test/functional/CredentialsIT.java > 8e2e1e0 > > test/src/test/java/org/apache/accumulo/test/functional/DeleteEverythingIT.java > 0578ef4 > test/src/test/java/org/apache/accumulo/test/functional/DeleteIT.java > 3510fbd > test/src/test/java/org/apache/accumulo/test/functional/DeleteRowsIT.java > 4b7d664 > > test/src/test/java/org/apache/accumulo/test/functional/DeleteRowsSplitIT.java > d35ba9f > > test/src/test/java/org/apache/accumulo/test/functional/DeleteTableDuringSplitIT.java > a0bff64 > > test/src/test/java/org/apache/accumulo/test/functional/DynamicThreadPoolsIT.java > 87497b9 > > test/src/test/java/org/apache/accumulo/test/functional/FateStarvationIT.java > 4d75a16 > test/src/test/java/org/apache/accumulo/test/functional/LargeRowIT.java > d77d060 > test/src/test/java/org/apache/accumulo/test/functional/LogicalTimeIT.java > 6aec7cd > > test/src/test/java/org/apache/accumulo/test/functional/MasterAssignmentIT.java > 46f6b23 > > test/src/test/java/org/apache/accumulo/test/functional/MasterFailoverIT.java > 218d65e > test/src/test/java/org/apache/accumulo/test/functional/MaxOpenIT.java > 2649890 > test/src/test/java/org/apache/accumulo/test/functional/MergeIT.java c264dfe > test/src/test/java/org/apache/accumulo/test/functional/MetadataIT.java > bd0282d > > test/src/test/java/org/apache/accumulo/test/functional/MetadataMaxFiles.java > 6b8d9b3 > > test/src/test/java/org/apache/accumulo/test/functional/MetadataMaxFilesIT.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/functional/PermissionsIT.java > d51dcbb > test/src/test/java/org/apache/accumulo/test/functional/ReadWriteIT.java > 60b1908 > > test/src/test/java/org/apache/accumulo/test/functional/RecoveryWithEmptyRFileIT.java > 814dd85 > test/src/test/java/org/apache/accumulo/test/functional/RenameIT.java > 8cbe84f > test/src/test/java/org/apache/accumulo/test/functional/RestartIT.java > e4b9c5a > test/src/test/java/org/apache/accumulo/test/functional/RestartStressIT.java > d8c2804 > test/src/test/java/org/apache/accumulo/test/functional/RowDeleteIT.java > 4dbd912 > test/src/test/java/org/apache/accumulo/test/functional/ScanIteratorIT.java > 189a55c > test/src/test/java/org/apache/accumulo/test/functional/ScanRangeIT.java > 90b881c > > test/src/test/java/org/apache/accumulo/test/functional/ScanSessionTimeOutIT.java > 3547b68 > test/src/test/java/org/apache/accumulo/test/functional/ScannerIT.java > cbd1290 > > test/src/test/java/org/apache/accumulo/test/functional/ServerSideErrorIT.java > d765b16 > test/src/test/java/org/apache/accumulo/test/functional/SimpleMacIT.java > a4e6647 > > test/src/test/java/org/apache/accumulo/test/functional/SparseColumnFamilyIT.java > 0b63d01 > test/src/test/java/org/apache/accumulo/test/functional/SplitIT.java 6203523 > test/src/test/java/org/apache/accumulo/test/functional/SslIT.java a14795c > test/src/test/java/org/apache/accumulo/test/functional/StartIT.java 82278af > test/src/test/java/org/apache/accumulo/test/functional/TableIT.java 832ec60 > test/src/test/java/org/apache/accumulo/test/functional/TabletIT.java > fccc79f > test/src/test/java/org/apache/accumulo/test/functional/TimeoutIT.java > 4dc72e0 > test/src/test/java/org/apache/accumulo/test/functional/VisibilityIT.java > f2460cf > test/src/test/java/org/apache/accumulo/test/functional/WriteAheadLogIT.java > af6eca5 > test/src/test/java/org/apache/accumulo/test/functional/WriteLotsIT.java > 214fc2f > > Diff: https://reviews.apache.org/r/28214/diff/ > > > Testing > ------- > > Haven't had a 100% IT pass rate yet (last run was about 95% pass rate), but I > wanted to get the code up and have some eyes on it sooner than later. > > > Thanks, > > Josh Elser > >
