accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 28214: Decouple MiniAccumuloCluster from integration tests and introduce StandaloneAccumuloCluster
Date Wed, 19 Nov 2014 17:00:00 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message