Re: Review Request 28214: Decouple MiniAccumuloCluster from integration tests and introduce StandaloneAccumuloCluster

[keith]

> On Nov. 21, 2014, 3:19 a.m., kturner wrote:
> > test/src/test/java/org/apache/accumulo/test/ConditionalWriterIT.java, line 
> > 98
> > 
> >
> > SimpleMacIT [starts one MAC][1] per class.  It seems like 
> > AccumuloClusterIT starts a MAC per method?   
> > 
> > 
> > [1]:https://github.com/apache/accumulo/blob/1.6.1/test/src/test/java/org/apache/accumulo/test/functional/SimpleMacIT.java#L60
> 
> Josh Elser wrote:
> Yes, this is a change. I made SharedMiniClusterIT to make ShellServerIT 
> run a *lot* faster, I didn't see any other ITs that were affected to the same 
> extent. I couldn't come up with a good way to implement this inside of 
> AccumuloClusterIT so I punted on it for now. Suggestions welcome.
> 
> kturner wrote:
> Ok.  I saw SharedMiniClusterIT and was wondering why only ShellServerIT 
> used it.  Do you know what the difference in build times is w/ this change?
> 
> Josh Elser wrote:
> Ha, significant. Up to 230s instead of 30 when running with MAC :). Will 
> see if I can come up with anything to address it.
> 
> kturner wrote:
> > Ha, significant. Up to 230s instead of 30 when running with MAC :).
> 
> Thats for an individual test.  Do you know what the total diff for `time 
> mvn verify` is w/ and w/o your changes?
> 
> Josh Elser wrote:
> Oh, sorry, I thought you were only asking specifically about 
> ConditionalWriterIT :) (with a standalone instance, it's 25s if you were 
> curious :P). I'll run some comparisions against 1.6 presently, 1.6 with these 
> changes out of the box (still using MAC) and then 1.6 with a standalone 
> instance available tonight.
> 
> Josh Elser wrote:
> Oddly, NamespacesIT has also been really problematic with ZK starting. My 
> guess is that it this change is also affecting things for that test. 
> Definitely something I need to look into until we resolve the ZK issue.
> 
> kturner wrote:
> > with a standalone instance, it's 25s if you were curious
> 
> Do you think we could start a Mini instance preintegration test and use 
> that as a "standalone instance", if no standalone instance is configured?
> 
> Josh Elser wrote:
> > Do you think we could start a Mini instance preintegration test and use 
> that as a "standalone instance", if no standalone instance is configured?
> 
> We would have to make some sort of new cluster to support this. The 
> start/stop semantics (using start-server.sh) would not work to use a normal 
> StandaloneCluster with MAC. This wouldn't be very difficult though.
> 
> > Do you know what the total diff for time mvn verify is w/ and w/o your 
> changes?
> 
> Summary of baseline (before these changes), running against MAC with 
> changes, and then using a Standalone single-node cluster (falling back to MAC 
> when the test doesn't support the standalone test): 
> http://people.apache.org/~elserj/accumulo/ACCUMULO-3167-comparison.html. The 
> only thing I did special to the configuration of the StandaloneCluster was 
> setting instance.zookeeper.timeout=15s instead of the default 30s.

Nice report.   So w/ these changes `mvn verify` took ~11 more mins, and 
increase of ~11%.  These are a nice set of changes, IMO nothing needs to be 
done about the 11% increase.


- kturner


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28214/#review62514
---


On Nov. 21, 2014, 8:40 p.m., Josh Elser wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28214/
> ---
> 
> (Updated Nov. 21, 2014, 8:40 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 hel

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Mike Drob]

> On Nov. 21, 2014, 11:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, 
> > lines 57-64
> > 
> >
> > Is this class meant to be thread-safe? These two methods are 
> > synchronized, but the rest of them aren't.
> 
> Christopher Tubbs wrote:
> This method is the only place where we allow setting a member field after 
> constructing, and the context is passed around a lot, including in internal 
> threads, so yes, I want it to be thread-safe. However, it may not really 
> matter, since the only place this is actually used is in the shell and 
> SecurityOperationsImpl, and we only place the serialized credentials in the 
> RPC thread pools. Much of what it replaces was already synchronized 
> (synchronized access to HdfsZooInstance.getInstance() singleton, for 
> example), so synchronization here isn't any different from what we had 
> before. I think we can defer this to future analysis, unless a compelling 
> reason can be made now to go one way or the other.

If we're going to defer things to the future, I'd feel more comfortable with a 
TODO and an open JIRA to make sure it doesn't get lost.


> On Nov. 21, 2014, 11:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java,
> >  lines 91-99
> > 
> >
> > Are these doing the same thing? This looks like a big change.
> 
> Christopher Tubbs wrote:
> Yes, they were doing the same thing. Look at InstanceOperationsImpl. This 
> was simply removing redundant code.

Can we split this out into a separate issue? It muddles the changeset here.


> On Nov. 21, 2014, 11:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, 
> > lines 86-91
> > 
> >
> > We could make this a utility method somewhere else. I'd really rather 
> > not see something "for internal use only" on the new face of our public API.
> 
> Christopher Tubbs wrote:
> This isn't public API. Nothing in this patch changes public API. The 
> entire class is internal use only. So, the javadoc is redundant. I can remove 
> the javadoc when I add a javadoc for the whole class, but it doesn't really 
> matter.

Is this change setting us up for making future changes to the public API? I was 
initially under the impression that it decreases our public API surface going 
forward, but if it's not making those changes then I don't immediately see the 
benefits introduced.


- Mike


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62669
---


On Nov. 22, 2014, 2:21 a.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 22, 2014, 2:21 a.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> These new object types also reduce the need to create multiple references to
> commonly used internal objects (such as HdfsZooInstance and
> SystemCredentials), and avoids storing information in static fields. As a
> side-effect, this should allow for better testing with mocked components.
> 
> This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 
> 4e11a14 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java 
> bd76a50 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
>  603172f 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
> PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
> 

Re: Review Request 28214: Decouple MiniAccumuloCluster from integration tests and introduce StandaloneAccumuloCluster

[Josh Elser]

> On Nov. 21, 2014, 3:19 a.m., kturner wrote:
> > test/src/test/java/org/apache/accumulo/test/ConditionalWriterIT.java, line 
> > 98
> > 
> >
> > SimpleMacIT [starts one MAC][1] per class.  It seems like 
> > AccumuloClusterIT starts a MAC per method?   
> > 
> > 
> > [1]:https://github.com/apache/accumulo/blob/1.6.1/test/src/test/java/org/apache/accumulo/test/functional/SimpleMacIT.java#L60
> 
> Josh Elser wrote:
> Yes, this is a change. I made SharedMiniClusterIT to make ShellServerIT 
> run a *lot* faster, I didn't see any other ITs that were affected to the same 
> extent. I couldn't come up with a good way to implement this inside of 
> AccumuloClusterIT so I punted on it for now. Suggestions welcome.
> 
> kturner wrote:
> Ok.  I saw SharedMiniClusterIT and was wondering why only ShellServerIT 
> used it.  Do you know what the difference in build times is w/ this change?
> 
> Josh Elser wrote:
> Ha, significant. Up to 230s instead of 30 when running with MAC :). Will 
> see if I can come up with anything to address it.
> 
> kturner wrote:
> > Ha, significant. Up to 230s instead of 30 when running with MAC :).
> 
> Thats for an individual test.  Do you know what the total diff for `time 
> mvn verify` is w/ and w/o your changes?
> 
> Josh Elser wrote:
> Oh, sorry, I thought you were only asking specifically about 
> ConditionalWriterIT :) (with a standalone instance, it's 25s if you were 
> curious :P). I'll run some comparisions against 1.6 presently, 1.6 with these 
> changes out of the box (still using MAC) and then 1.6 with a standalone 
> instance available tonight.
> 
> Josh Elser wrote:
> Oddly, NamespacesIT has also been really problematic with ZK starting. My 
> guess is that it this change is also affecting things for that test. 
> Definitely something I need to look into until we resolve the ZK issue.
> 
> kturner wrote:
> > with a standalone instance, it's 25s if you were curious
> 
> Do you think we could start a Mini instance preintegration test and use 
> that as a "standalone instance", if no standalone instance is configured?
> 
> Josh Elser wrote:
> > Do you think we could start a Mini instance preintegration test and use 
> that as a "standalone instance", if no standalone instance is configured?
> 
> We would have to make some sort of new cluster to support this. The 
> start/stop semantics (using start-server.sh) would not work to use a normal 
> StandaloneCluster with MAC. This wouldn't be very difficult though.
> 
> > Do you know what the total diff for time mvn verify is w/ and w/o your 
> changes?
> 
> Summary of baseline (before these changes), running against MAC with 
> changes, and then using a Standalone single-node cluster (falling back to MAC 
> when the test doesn't support the standalone test): 
> http://people.apache.org/~elserj/accumulo/ACCUMULO-3167-comparison.html. The 
> only thing I did special to the configuration of the StandaloneCluster was 
> setting instance.zookeeper.timeout=15s instead of the default 30s.
> 
> kturner wrote:
> Nice report.   So w/ these changes `mvn verify` took ~11 more mins, and 
> increase of ~11%.  These are a nice set of changes, IMO nothing needs to be 
> done about the 11% increase.

Thanks, Keith. I think I might be able to do something which makes those 
faster, but my plan is to try that after this behemoth gets committed :)


- Josh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28214/#review62514
---


On Nov. 21, 2014, 8:40 p.m., Josh Elser wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28214/
> ---
> 
> (Updated Nov. 21, 2014, 8:40 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 inte

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Josh Elser]

> On Nov. 21, 2014, 11:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, 
> > lines 86-91
> > 
> >
> > We could make this a utility method somewhere else. I'd really rather 
> > not see something "for internal use only" on the new face of our public API.
> 
> Christopher Tubbs wrote:
> This isn't public API. Nothing in this patch changes public API. The 
> entire class is internal use only. So, the javadoc is redundant. I can remove 
> the javadoc when I add a javadoc for the whole class, but it doesn't really 
> matter.
> 
> Mike Drob wrote:
> Is this change setting us up for making future changes to the public API? 
> I was initially under the impression that it decreases our public API surface 
> going forward, but if it's not making those changes then I don't immediately 
> see the benefits introduced.

I believe the point of this change is that, internally, when the implementation 
of Instance or Connector (at least those are the most common "client" cases) 
needs to communicate with a server, the TCredentials that may (or may not) need 
to be sent over the wire with the RPC are now encapsulated in one place. I also 
see this as beneficial because we're doing the same thing with the SSL 
configuration -- done in one place.


> On Nov. 21, 2014, 11:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java,
> >  lines 91-99
> > 
> >
> > Are these doing the same thing? This looks like a big change.
> 
> Christopher Tubbs wrote:
> Yes, they were doing the same thing. Look at InstanceOperationsImpl. This 
> was simply removing redundant code.
> 
> Mike Drob wrote:
> Can we split this out into a separate issue? It muddles the changeset 
> here.

As the dude who wrote this goofy bit of code (asking a the master to change a 
property instead of just changing the property through the API), I don't think 
this needs to be split into its own issue. My $0.02.


- Josh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62669
---


On Nov. 22, 2014, 2:21 a.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 22, 2014, 2:21 a.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> These new object types also reduce the need to create multiple references to
> commonly used internal objects (such as HdfsZooInstance and
> SystemCredentials), and avoids storing information in static fields. As a
> side-effect, this should allow for better testing with mocked components.
> 
> This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 
> 4e11a14 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java 
> bd76a50 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
>  603172f 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
> PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
>  cb0b90e 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
> 2d077d9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
>  9848612 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
> 53f178a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
>  b09582a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
>  f756ef1 
>   
> core/src/main/java/org/apache/acc

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Mike Drob]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62775
---



core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocator.java


Why did this method get an Instance argument added? It looks unused.



core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java


In some places you make the ClientContext argument final, in others not. I 
don't care either way, but would prefer consistency.



core/src/test/java/org/apache/accumulo/core/client/impl/ClientContextTest.java


Use org.junit.Assume here (and elsewhere in the test).



minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterStartStopTest.java


This looks unrelated. Split into a separate patch?



server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java


Why is this just an Instance instead of a Context?



server/base/src/main/java/org/apache/accumulo/server/master/state/TabletStateStore.java


This could be refactored into a separate method since it's used multiple 
times. Probably a separate issue.


Got through page 4. In general, I think we need more developer oriented 
documentation about when you should be passing a context and when it is safe to 
use a raw instance/connector/configuration.

- Mike Drob


On Nov. 22, 2014, 2:21 a.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 22, 2014, 2:21 a.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> These new object types also reduce the need to create multiple references to
> commonly used internal objects (such as HdfsZooInstance and
> SystemCredentials), and avoids storing information in static fields. As a
> side-effect, this should allow for better testing with mocked components.
> 
> This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 
> 4e11a14 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java 
> bd76a50 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
>  603172f 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
> PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
>  cb0b90e 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
> 2d077d9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
>  9848612 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
> 53f178a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
>  b09582a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
>  f756ef1 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
>  901128b 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
>  e45bcc9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/RootTabletLocator.java
>  97d476b 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 
> 670782e 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ScannerIterator.java 
> b0bfc20 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/SecurityOperationsImpl.java
>  8a17a77 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ServerClient.java 
> 2d

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Christopher Tubbs]

> On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, 
> > lines 57-64
> > 
> >
> > Is this class meant to be thread-safe? These two methods are 
> > synchronized, but the rest of them aren't.
> 
> Christopher Tubbs wrote:
> This method is the only place where we allow setting a member field after 
> constructing, and the context is passed around a lot, including in internal 
> threads, so yes, I want it to be thread-safe. However, it may not really 
> matter, since the only place this is actually used is in the shell and 
> SecurityOperationsImpl, and we only place the serialized credentials in the 
> RPC thread pools. Much of what it replaces was already synchronized 
> (synchronized access to HdfsZooInstance.getInstance() singleton, for 
> example), so synchronization here isn't any different from what we had 
> before. I think we can defer this to future analysis, unless a compelling 
> reason can be made now to go one way or the other.
> 
> Mike Drob wrote:
> If we're going to defer things to the future, I'd feel more comfortable 
> with a TODO and an open JIRA to make sure it doesn't get lost.

I can do that.


> On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, 
> > lines 86-91
> > 
> >
> > We could make this a utility method somewhere else. I'd really rather 
> > not see something "for internal use only" on the new face of our public API.
> 
> Christopher Tubbs wrote:
> This isn't public API. Nothing in this patch changes public API. The 
> entire class is internal use only. So, the javadoc is redundant. I can remove 
> the javadoc when I add a javadoc for the whole class, but it doesn't really 
> matter.
> 
> Mike Drob wrote:
> Is this change setting us up for making future changes to the public API? 
> I was initially under the impression that it decreases our public API surface 
> going forward, but if it's not making those changes then I don't immediately 
> see the benefits introduced.
> 
> Josh Elser wrote:
> I believe the point of this change is that, internally, when the 
> implementation of Instance or Connector (at least those are the most common 
> "client" cases) needs to communicate with a server, the TCredentials that may 
> (or may not) need to be sent over the wire with the RPC are now encapsulated 
> in one place. I also see this as beneficial because we're doing the same 
> thing with the SSL configuration -- done in one place.

This method nothing to do with public API. This was so we can do 
"context.rpcCreds()" instead of the longer 
"context.getCredentials().toThrift(context.getInstance)", or worse 
"context.getCredentials().toThrift(HdfsZooInstance.getInstance())", etc.

The entire changeset as a whole may help with the API changes in the future, if 
that API introduces additional information to the context that we need to carry 
around (additional timeouts, exception handlers, callbacks, whatever). The main 
benefit is that it allows us to ensure that we pass the right params where they 
are needed internally.


> On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java,
> >  lines 91-99
> > 
> >
> > Are these doing the same thing? This looks like a big change.
> 
> Christopher Tubbs wrote:
> Yes, they were doing the same thing. Look at InstanceOperationsImpl. This 
> was simply removing redundant code.
> 
> Mike Drob wrote:
> Can we split this out into a separate issue? It muddles the changeset 
> here.
> 
> Josh Elser wrote:
> As the dude who wrote this goofy bit of code (asking a the master to 
> change a property instead of just changing the property through the API), I 
> don't think this needs to be split into its own issue. My $0.02.

Is there a particular problem with this change that migth warrant additional 
review/consideration? If so, I'll break it out. As is, I see it as just one 
more bit of code that is simplified by the introduction of a context. 
Essentially, what you're asking me to do is modify this code to look like the 
following as an interim step, and I see no reason for this if it can be 
replaced with something that already does this:
MasterClient.execute(context.getInstance(), new ClientExec() {
  @Override
  public void execute(Client client) throws Exception {
client.setSystemProperty(Tracer.traceInfo(), context.rpcCreds(), 
Property.REPLICATION_PEERS.getKey() + name, replicaType);
  }
});


- Christopher


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/2831

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Mike Drob]

> On Nov. 21, 2014, 11:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java,
> >  lines 91-99
> > 
> >
> > Are these doing the same thing? This looks like a big change.
> 
> Christopher Tubbs wrote:
> Yes, they were doing the same thing. Look at InstanceOperationsImpl. This 
> was simply removing redundant code.
> 
> Mike Drob wrote:
> Can we split this out into a separate issue? It muddles the changeset 
> here.
> 
> Josh Elser wrote:
> As the dude who wrote this goofy bit of code (asking a the master to 
> change a property instead of just changing the property through the API), I 
> don't think this needs to be split into its own issue. My $0.02.
> 
> Christopher Tubbs wrote:
> Is there a particular problem with this change that migth warrant 
> additional review/consideration? If so, I'll break it out. As is, I see it as 
> just one more bit of code that is simplified by the introduction of a 
> context. Essentially, what you're asking me to do is modify this code to look 
> like the following as an interim step, and I see no reason for this if it can 
> be replaced with something that already does this:
> MasterClient.execute(context.getInstance(), new ClientExec() {
>   @Override
>   public void execute(Client client) throws Exception {
> client.setSystemProperty(Tracer.traceInfo(), context.rpcCreds(), 
> Property.REPLICATION_PEERS.getKey() + name, replicaType);
>   }
> });

I was actually suggesting the opposite - if the change does not actually depend 
on the introduction of a context, then you can apply it separately (and 
immediately).


- Mike


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62669
---


On Nov. 22, 2014, 2:21 a.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 22, 2014, 2:21 a.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> These new object types also reduce the need to create multiple references to
> commonly used internal objects (such as HdfsZooInstance and
> SystemCredentials), and avoids storing information in static fields. As a
> side-effect, this should allow for better testing with mocked components.
> 
> This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 
> 4e11a14 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java 
> bd76a50 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
>  603172f 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
> PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
>  cb0b90e 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
> 2d077d9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
>  9848612 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
> 53f178a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
>  b09582a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
>  f756ef1 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
>  901128b 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
>  e45bcc9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/RootTabletLocator.java
>  97d476b 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 
> 670782e 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl

Re: Updated Accumulo Benchmarking Paper

[Adam Fuchs]

Looks good to me. Another option would be to put multiple links under
one entry. That might be a bit cleaner.

Adam

On Sun, Nov 23, 2014 at 10:15 PM, Josh Elser  wrote:
> +1 publish that sucker. Thanks for updating the site with the new paper.
>
>
> Drew Farris wrote:
>>
>> Ok, http://accumulo.staging.apache.org/papers.html is updated with a
>> reference to the updated paper, I'll publish tomorrow if there are no
>> objections..
>>
>> On Sun, Nov 23, 2014 at 9:32 AM, Drew Farris
>> wrote:
>>
>>> Yes, I can't claim this version is peer reviewed. A second link is like a
>>> great idea. I'll get the change up on staging.
>>>
>>> Thanks for the feedback.
>>>
>>> On Sat, Nov 22, 2014 at 6:50 PM, Sean Busbey  wrote:
>>>
 I think it's worth maintaining a pointer to the peer reviewed version.

 How about leave the current link with the IEEE and peer reviewed bits,
 then
 add a follow on entry that bears teh note "updated to include foo" that
 does not have the peer-reviewed marker?

 On Sat, Nov 22, 2014 at 5:09 PM, Jeremy Kepner
 wrote:

> So the only question would be how you would fill out the
> "Peer-Reviewed"
> column.
>
> On Sat, Nov 22, 2014 at 05:46:47PM -0500, Drew Farris wrote:
>>
>> Hi All,
>>
>> There is an updated version of the paper "Benchmarking Apache Accumulo
>> BigData Distributed Table Store Using Its Continuous Test Suite"

 that's
>>
>> currently linked in the papers section of the Accumulo website.
>>
>> Would anyone object to me updating the link? I believe I have the
>
> necessary
>>
>> karma to do so, but was reluctant to do so without first informing the
>> community of my intent..
>>
>> This version has been edited for clarity, structure and includes

 results
>>
>> from running the tests described on the paper on Amazon EC2. It no

 longer
>>
>> bears the IEEE Big Data mark.
>>
>> The new link is http://sqrrl.com/media/accumulo-benchmarking-2.1.pdf
>>
>> Drew



 --
 Sean

>>>
>>
>

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Christopher Tubbs]

> On Nov. 24, 2014, 10:45 a.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java,
> >  line 110
> > 
> >
> > In some places you make the ClientContext argument final, in others 
> > not. I don't care either way, but would prefer consistency.

Yes, I started doing final everywhere... and then forgot in a few places. I can 
make them consistent, but it doesn't affect the behavior at this point, so I 
can polish that up after the review.


> On Nov. 24, 2014, 10:45 a.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TabletLocator.java, 
> > line 65
> > 
> >
> > Why did this method get an Instance argument added? It looks unused.

It's required for the RootTabletLocator, and it's needed because the Instance 
is not passed in when it is constructed anymore, because there is no single 
context in which the locator cache is used. It's stored statically and shared 
among all contexts. This is why Credentials were passed in previously to the 
individual method calls (which I replaced with a context). Replacing this one 
with a context would have introduced a much larger changeset because of 
testing. Personally, I feel the TabletLocator stuff needs significant work to 
improve it, but this was the minimal change I could do to keep the current 
behavior and tests, in a way that made sense with context.


> On Nov. 24, 2014, 10:45 a.m., Mike Drob wrote:
> > core/src/test/java/org/apache/accumulo/core/client/impl/ClientContextTest.java,
> >  lines 79-81
> > 
> >
> > Use org.junit.Assume here (and elsewhere in the test).

Existing code. I'm not altering the test that already existed. This test class 
was simply renamed.


> On Nov. 24, 2014, 10:45 a.m., Mike Drob wrote:
> > minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterStartStopTest.java,
> >  lines 41-46
> > 
> >
> > This looks unrelated. Split into a separate patch?

I made this change while trying to make sense of test failures while debugging. 
I don't see value in undoing test improvements that helped me debug this. I 
think the change is also done upstream, and I'll have to address on merge 
anyway.


> On Nov. 24, 2014, 10:45 a.m., Mike Drob wrote:
> > server/base/src/main/java/org/apache/accumulo/server/conf/TableConfiguration.java,
> >  line 46
> > 
> >
> > Why is this just an Instance instead of a Context?

Because the dependency graph on server-side configuration is a ridiculous 
nightmare, but in short, the AccumuloServerContext depends on server-side 
configuration, so server-side configuration must not depend on context. There's 
room for improvement, but I have not even begun considering what kinds of 
things would help improve configuration on the server-side, and it's well 
outside the scope of this change.


> On Nov. 24, 2014, 10:45 a.m., Mike Drob wrote:
> > server/base/src/main/java/org/apache/accumulo/server/master/state/TabletStateStore.java,
> >  lines 63-70
> > 
> >
> > This could be refactored into a separate method since it's used 
> > multiple times. Probably a separate issue.

Yes, separate issue. This isn't part of my change (except the params).


- Christopher


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62775
---


On Nov. 21, 2014, 9:21 p.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 21, 2014, 9:21 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> 

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Christopher Tubbs]

> On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java,
> >  lines 91-99
> > 
> >
> > Are these doing the same thing? This looks like a big change.
> 
> Christopher Tubbs wrote:
> Yes, they were doing the same thing. Look at InstanceOperationsImpl. This 
> was simply removing redundant code.
> 
> Mike Drob wrote:
> Can we split this out into a separate issue? It muddles the changeset 
> here.
> 
> Josh Elser wrote:
> As the dude who wrote this goofy bit of code (asking a the master to 
> change a property instead of just changing the property through the API), I 
> don't think this needs to be split into its own issue. My $0.02.
> 
> Christopher Tubbs wrote:
> Is there a particular problem with this change that migth warrant 
> additional review/consideration? If so, I'll break it out. As is, I see it as 
> just one more bit of code that is simplified by the introduction of a 
> context. Essentially, what you're asking me to do is modify this code to look 
> like the following as an interim step, and I see no reason for this if it can 
> be replaced with something that already does this:
> MasterClient.execute(context.getInstance(), new ClientExec() {
>   @Override
>   public void execute(Client client) throws Exception {
> client.setSystemProperty(Tracer.traceInfo(), context.rpcCreds(), 
> Property.REPLICATION_PEERS.getKey() + name, replicaType);
>   }
> });
> 
> Mike Drob wrote:
> I was actually suggesting the opposite - if the change does not actually 
> depend on the introduction of a context, then you can apply it separately 
> (and immediately).

Either way, those lines of code would be modified with the introduction of 
context.


- Christopher


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62669
---


On Nov. 21, 2014, 9:21 p.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 21, 2014, 9:21 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> These new object types also reduce the need to create multiple references to
> commonly used internal objects (such as HdfsZooInstance and
> SystemCredentials), and avoids storing information in static fields. As a
> side-effect, this should allow for better testing with mocked components.
> 
> This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 
> 4e11a14 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java 
> bd76a50 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
>  603172f 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
> PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
>  cb0b90e 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
> 2d077d9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
>  9848612 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
> 53f178a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
>  b09582a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
>  f756ef1 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
>  901128b 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
>  e45bcc9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/RootTabletLocator.java
>  97d476b 
>   core/src/main/java/org/a

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Christopher Tubbs]

> On Nov. 21, 2014, 6:06 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java, 
> > lines 57-64
> > 
> >
> > Is this class meant to be thread-safe? These two methods are 
> > synchronized, but the rest of them aren't.
> 
> Christopher Tubbs wrote:
> This method is the only place where we allow setting a member field after 
> constructing, and the context is passed around a lot, including in internal 
> threads, so yes, I want it to be thread-safe. However, it may not really 
> matter, since the only place this is actually used is in the shell and 
> SecurityOperationsImpl, and we only place the serialized credentials in the 
> RPC thread pools. Much of what it replaces was already synchronized 
> (synchronized access to HdfsZooInstance.getInstance() singleton, for 
> example), so synchronization here isn't any different from what we had 
> before. I think we can defer this to future analysis, unless a compelling 
> reason can be made now to go one way or the other.
> 
> Mike Drob wrote:
> If we're going to defer things to the future, I'd feel more comfortable 
> with a TODO and an open JIRA to make sure it doesn't get lost.
> 
> Christopher Tubbs wrote:
> I can do that.

Upon further examination, and consulting with Keith Turner, I've convinced 
myself this synchronization is necessary. The user can change their credentials 
with the SecurityOperations API, and there could be other threads in the client 
that are using the old credentials. This can result in an inconsistent view of 
the current user's credentials and could cause some RPC calls to succeed while 
others fail, for extended periods of time. So, yes, the synchronization is 
necessary.


- Christopher


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62669
---


On Nov. 21, 2014, 9:21 p.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 21, 2014, 9:21 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> These new object types also reduce the need to create multiple references to
> commonly used internal objects (such as HdfsZooInstance and
> SystemCredentials), and avoids storing information in static fields. As a
> side-effect, this should allow for better testing with mocked components.
> 
> This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 
> 4e11a14 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java 
> bd76a50 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
>  603172f 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
> PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
>  cb0b90e 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
> 2d077d9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
>  9848612 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
> 53f178a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
>  b09582a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
>  f756ef1 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
>  901128b 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
>  e45bcc9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/RootTabletLocator.java
>  97d476b 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ScannerImpl.java 
> 670782e 

Re: Dropping Hadoop 1 for 1.7.0

[Christopher]

Did we make a decision on this?


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Sun, Nov 16, 2014 at 11:16 PM, Christopher  wrote:

> On Sun, Nov 16, 2014 at 8:21 PM, Josh Elser  wrote:
>
>> Have we already decided to drop Hadoop 1 for 1.7.0? I can't remember
>> anymore.
>>
>> If we haven't decided to do so already, I'd like to suggest doing so.
>>
>> - Josh
>>
>
> I thought we had discussed this for 2.0.0, but that was when we thought we
> might release a 2.0 instead of a 1.7. I'm okay with doing it in 1.7, but
> with some minor reservations. Doing this will mean that the last supported
> Accumulo line for Hadoop 1 users is 1.6.x. This will mean that if we want
> to support users on Hadoop 1, we'll have to maintain the 1.6.x branch
> longer, vs. maintaining support with 1.x (1.7, maybe 1.8, etc.)
>
> So, doing this will involve a choice in the future: continue maintaining
> 1.6 for perhaps longer than we might have otherwise done, or leave those
> users in the dust (if there are any).
>
> --
> Christopher L Tubbs II
> http://gravatar.com/ctubbsii
>
>

Re: Review Request 21826: Accumulo-2632

[John McNamee]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21826/
---

(Updated Nov. 24, 2014, 1:26 p.m.)


Review request for accumulo.


Changes
---

Link to the JIRA issue.


Bugs: ACCUMULO-2632
https://issues.apache.org/jira/browse/ACCUMULO-2632


Repository: accumulo


Description
---

The goal was to find a way to increase the write speed in the DfsLogger using 
compression.
This patch uses the Snappy compression library and was the most optimum way 
that I could find to increase write performance.
The option to use compression has been made configurable.
Recovery has been modified to uncompress the WAL if needed.


Diffs
-

  core/src/main/java/org/apache/accumulo/core/conf/Property.java 62b0a33 
  
server/tserver/src/main/java/org/apache/accumulo/tserver/log/DecompressingInputStream.java
 PRE-CREATION 
  server/tserver/src/main/java/org/apache/accumulo/tserver/log/DfsLogger.java 
bfa51d3 
  
server/tserver/src/test/java/org/apache/accumulo/tserver/log/DecompressingInputStreamTest.java
 PRE-CREATION 

Diff: https://reviews.apache.org/r/21826/diff/


Testing
---

There is a unit test for the DecryptingInputStream.java, and I ran continuous 
Ingest/ continuous verify with the agitator on.


Thanks,

John McNamee

Re: Review Request 15846: Prototype implementation of ACCUMULO-1787.

[Chris McCubbin]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15846/
---

(Updated Nov. 24, 2014, 1:31 p.m.)


Review request for accumulo, Adam Fuchs, Josh Elser, and kturner.


Changes
---

Link to the JIRA issue.


Bugs: ACCUMULO-1787
https://issues.apache.org/jira/browse/ACCUMULO-1787


Repository: accumulo


Description
---

This is a prototype implementation of ACCUMULO-1787.


Diffs
-

  core/src/main/java/org/apache/accumulo/core/conf/Property.java f4c1778 
  server/src/main/java/org/apache/accumulo/server/tabletserver/Compactor.java 
8e4af64 

Diff: https://reviews.apache.org/r/15846/diff/


Testing
---


Thanks,

Chris McCubbin

Re: Dropping Hadoop 1 for 1.7.0

[Sean Busbey]

I think a little over a week is a fair window and AFAICT we have lazy
consensus to drop it.

-Sean


On Mon, Nov 24, 2014 at 12:07 PM, Christopher  wrote:

> Did we make a decision on this?
>
>
> --
> Christopher L Tubbs II
> http://gravatar.com/ctubbsii
>
> On Sun, Nov 16, 2014 at 11:16 PM, Christopher  wrote:
>
> > On Sun, Nov 16, 2014 at 8:21 PM, Josh Elser 
> wrote:
> >
> >> Have we already decided to drop Hadoop 1 for 1.7.0? I can't remember
> >> anymore.
> >>
> >> If we haven't decided to do so already, I'd like to suggest doing so.
> >>
> >> - Josh
> >>
> >
> > I thought we had discussed this for 2.0.0, but that was when we thought
> we
> > might release a 2.0 instead of a 1.7. I'm okay with doing it in 1.7, but
> > with some minor reservations. Doing this will mean that the last
> supported
> > Accumulo line for Hadoop 1 users is 1.6.x. This will mean that if we want
> > to support users on Hadoop 1, we'll have to maintain the 1.6.x branch
> > longer, vs. maintaining support with 1.x (1.7, maybe 1.8, etc.)
> >
> > So, doing this will involve a choice in the future: continue maintaining
> > 1.6 for perhaps longer than we might have otherwise done, or leave those
> > users in the dust (if there are any).
> >
> > --
> > Christopher L Tubbs II
> > http://gravatar.com/ctubbsii
> >
> >
>



-- 
Sean

Re: Review Request 26572: ACCUMULO-898

[Eric Newton]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26572/
---

(Updated Nov. 24, 2014, 1:48 p.m.)


Review request for accumulo.


Changes
---

Link to JIRA issue.


Bugs: ACCUMULO-898
https://issues.apache.org/jira/browse/ACCUMULO-898


Repository: accumulo


Description
---

Switch to HTrace


Diffs
-

  core/pom.xml 086ebc0 
  core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java e582160 
  
core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
 e8af187 
  core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
62ab6a4 
  
core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
 c5f7634 
  
core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
 7087ac5 
  
core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
 f820aa4 
  
core/src/main/java/org/apache/accumulo/core/client/impl/SecurityOperationsImpl.java
 e9c057e 
  
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
 e46b9c9 
  
core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java
 d2ca60e 
  
core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java
 5eec397 
  core/src/main/java/org/apache/accumulo/core/client/impl/ThriftScanner.java 
c49330e 
  core/src/main/java/org/apache/accumulo/core/client/impl/Writer.java 0358a88 
  core/src/main/java/org/apache/accumulo/core/trace/DistributedTrace.java 
83f5c26 
  core/src/main/java/org/apache/accumulo/core/trace/SpanTree.java 772a133 
  core/src/main/java/org/apache/accumulo/core/trace/TraceDump.java b44cc3e 
  core/src/main/java/org/apache/accumulo/core/trace/TraceFormatter.java 9d860d9 
  core/src/main/java/org/apache/accumulo/core/trace/ZooTraceClient.java 4c8b837 
  core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java da4e567 
  
examples/simple/src/main/java/org/apache/accumulo/examples/simple/client/TracingExample.java
 a542263 
  
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloClusterImpl.java
 1fb5901 
  pom.xml ebc2f2f 
  server/base/pom.xml 60762be 
  server/base/src/main/java/org/apache/accumulo/server/Accumulo.java 7bb3d71 
  server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java 
7097079 
  
server/base/src/main/java/org/apache/accumulo/server/master/LiveTServerSet.java 
3dbc6ba 
  
server/base/src/main/java/org/apache/accumulo/server/master/balancer/TabletBalancer.java
 59d3e0b 
  
server/base/src/main/java/org/apache/accumulo/server/trace/TraceFSDataInputStream.java
 5162e01 
  
server/base/src/main/java/org/apache/accumulo/server/trace/TraceFileSystem.java 
d3fbad7 
  server/base/src/main/java/org/apache/accumulo/server/util/Admin.java d85d61e 
  
server/base/src/main/java/org/apache/accumulo/server/util/VerifyTabletAssignments.java
 0fbeb6b 
  
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectWriteAheadLogs.java
 9646be9 
  
server/gc/src/main/java/org/apache/accumulo/gc/GarbageCollectionAlgorithm.java 
0f5cada 
  server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java 
10fd7f5 
  
server/gc/src/main/java/org/apache/accumulo/gc/replication/CloseWriteAheadLogReferences.java
 74da72d 
  server/master/src/main/java/org/apache/accumulo/master/Master.java b435b0f 
  
server/master/src/main/java/org/apache/accumulo/master/metrics/ReplicationMetrics.java
 06a653d 
  
server/master/src/main/java/org/apache/accumulo/master/replication/ReplicationDriver.java
 a52f743 
  
server/master/src/main/java/org/apache/accumulo/master/replication/StatusMaker.java
 68dd005 
  
server/master/src/main/java/org/apache/accumulo/master/replication/WorkMaker.java
 da17bba 
  
server/master/src/main/java/org/apache/accumulo/master/tableOps/BulkImport.java 
b55b315 
  
server/master/src/main/java/org/apache/accumulo/master/tableOps/TraceRepo.java 
9b1a735 
  server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java 7a724f8 
  
server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/TServersServlet.java
 452f790 
  
server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/trace/ShowTrace.java
 a476201 
  
server/monitor/src/test/java/org/apache/accumulo/monitor/ShowTraceLinkTypeTest.java
 a630434 
  server/tracer/src/main/java/org/apache/accumulo/tracer/TraceServer.java 
189bb39 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 
2e1eb2c 
  
server/tserver/src/main/java/org/apache/accumulo/tserver/mastermessage/SplitReportMessage.java
 2eb1077 
  
server/tserver/src/main/java/org/apache/accumulo/tserver/mastermessage/TabletStatusMessage.java
 b168625 
  
server/tserver/src/main/java/org/apache/accumulo/tserver/replication/AccumuloReplicaSystem.java
 4079a65 
  

Re: Dropping Hadoop 1 for 1.7.0

[Josh Elser]

Ya -- I've been holding this in my back pocket.

I'll open up an issue on JIRA later today if we don't already have one.

Sean Busbey wrote:

I think a little over a week is a fair window and AFAICT we have lazy
consensus to drop it.

-Sean


On Mon, Nov 24, 2014 at 12:07 PM, Christopher mailto:ctubb...@apache.org>> wrote:

Did we make a decision on this?


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Sun, Nov 16, 2014 at 11:16 PM, Christopher mailto:ctubb...@apache.org>> wrote:

 > On Sun, Nov 16, 2014 at 8:21 PM, Josh Elser mailto:josh.el...@gmail.com>> wrote:
 >
 >> Have we already decided to drop Hadoop 1 for 1.7.0? I can't remember
 >> anymore.
 >>
 >> If we haven't decided to do so already, I'd like to suggest
doing so.
 >>
 >> - Josh
 >>
 >
 > I thought we had discussed this for 2.0.0, but that was when we
thought we
 > might release a 2.0 instead of a 1.7. I'm okay with doing it in
1.7, but
 > with some minor reservations. Doing this will mean that the last
supported
 > Accumulo line for Hadoop 1 users is 1.6.x. This will mean that if
we want
 > to support users on Hadoop 1, we'll have to maintain the 1.6.x branch
 > longer, vs. maintaining support with 1.x (1.7, maybe 1.8, etc.)
 >
 > So, doing this will involve a choice in the future: continue
maintaining
 > 1.6 for perhaps longer than we might have otherwise done, or
leave those
 > users in the dust (if there are any).
 >
 > --
 > Christopher L Tubbs II
 > http://gravatar.com/ctubbsii
 >
 >




--
Sean

Re: Review Request 27829: ACCUMULO-1085

[Eric Newton]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27829/
---

(Updated Nov. 24, 2014, 1:53 p.m.)


Review request for accumulo.


Changes
---

Add link to JIRA issue.


Bugs: ACCUMULO-1085
https://issues.apache.org/jira/browse/ACCUMULO-1085


Repository: accumulo


Description
---

Allow for multiple threads to perform assignment, but use only a single thread 
for recovery playback


Diffs
-

  core/src/main/java/org/apache/accumulo/core/conf/Property.java f59b654 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 
1e81947 
  
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java
 ba86522 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java 
9490903 
  test/src/test/java/org/apache/accumulo/test/AssignmentThreadsIT.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/27829/diff/


Testing
---

Added an IT that verifies that tablet loading goes faster with more threads.


Thanks,

Eric Newton

Re: Dropping Hadoop 1 for 1.7.0

[Christopher]

Okay.


--
Christopher L Tubbs II
http://gravatar.com/ctubbsii

On Mon, Nov 24, 2014 at 1:40 PM, Josh Elser  wrote:

> Ya -- I've been holding this in my back pocket.
>
> I'll open up an issue on JIRA later today if we don't already have one.
>
> Sean Busbey wrote:
>
>> I think a little over a week is a fair window and AFAICT we have lazy
>> consensus to drop it.
>>
>> -Sean
>>
>>
>> On Mon, Nov 24, 2014 at 12:07 PM, Christopher > > wrote:
>>
>> Did we make a decision on this?
>>
>>
>> --
>> Christopher L Tubbs II
>> http://gravatar.com/ctubbsii
>>
>> On Sun, Nov 16, 2014 at 11:16 PM, Christopher > > wrote:
>>
>>  > On Sun, Nov 16, 2014 at 8:21 PM, Josh Elser > > wrote:
>>  >
>>  >> Have we already decided to drop Hadoop 1 for 1.7.0? I can't
>> remember
>>  >> anymore.
>>  >>
>>  >> If we haven't decided to do so already, I'd like to suggest
>> doing so.
>>  >>
>>  >> - Josh
>>  >>
>>  >
>>  > I thought we had discussed this for 2.0.0, but that was when we
>> thought we
>>  > might release a 2.0 instead of a 1.7. I'm okay with doing it in
>> 1.7, but
>>  > with some minor reservations. Doing this will mean that the last
>> supported
>>  > Accumulo line for Hadoop 1 users is 1.6.x. This will mean that if
>> we want
>>  > to support users on Hadoop 1, we'll have to maintain the 1.6.x
>> branch
>>  > longer, vs. maintaining support with 1.x (1.7, maybe 1.8, etc.)
>>  >
>>  > So, doing this will involve a choice in the future: continue
>> maintaining
>>  > 1.6 for perhaps longer than we might have otherwise done, or
>> leave those
>>  > users in the dust (if there are any).
>>  >
>>  > --
>>  > Christopher L Tubbs II
>>  > http://gravatar.com/ctubbsii
>>  >
>>  >
>>
>>
>>
>>
>> --
>> Sean
>>
>

Re: Review Request 27829: ACCUMULO-1085

[Eric Newton]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27829/
---

(Updated Nov. 24, 2014, 8:15 p.m.)


Review request for accumulo.


Bugs: ACCUMULO-1085
https://issues.apache.org/jira/browse/ACCUMULO-1085


Repository: accumulo


Description
---

Allow for multiple threads to perform assignment, but use only a single thread 
for recovery playback


Diffs (updated)
-

  core/src/main/java/org/apache/accumulo/core/conf/Property.java f59b654 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 
1e81947 
  
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java
 ba86522 
  server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java 
9490903 
  test/src/test/java/org/apache/accumulo/test/AssignmentThreadsIT.java 
PRE-CREATION 

Diff: https://reviews.apache.org/r/27829/diff/


Testing
---

Added an IT that verifies that tablet loading goes faster with more threads.


Thanks,

Eric Newton

Re: Review Request 27829: ACCUMULO-1085

[Eric Newton]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27829/
---

(Updated Nov. 24, 2014, 8:33 p.m.)


Review request for accumulo.


Bugs: ACCUMULO-1085
https://issues.apache.org/jira/browse/ACCUMULO-1085


Repository: accumulo


Description
---

Allow for multiple threads to perform assignment, but use only a single thread 
for recovery playback


Diffs (updated)
-

  core/src/main/java/org/apache/accumulo/core/conf/Property.java 1195668 
  server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 
93161ee 
  
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java
 ba86522 
  test/src/test/java/org/apache/accumulo/test/AssignmentThreadsIT.java 
PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/VerifySerialRecoveryIT.java 
PRE-CREATION 
  test/src/test/java/org/apache/accumulo/test/functional/BulkFileIT.java 
475d5cf 
  
test/src/test/java/org/apache/accumulo/test/functional/FunctionalTestUtils.java 
1246efe 

Diff: https://reviews.apache.org/r/27829/diff/


Testing
---

Added an IT that verifies that tablet loading goes faster with more threads.


Thanks,

Eric Newton

Re: Review Request 28312: ACCUMULO-3199 Internal refactor to add ClientContext

[Christopher Tubbs]

On Nov. 24, 2014, 10:45 a.m., Christopher Tubbs wrote:
> > Got through page 4. In general, I think we need more developer oriented 
> > documentation about when you should be passing a context and when it is 
> > safe to use a raw instance/connector/configuration.

Context replaces the need to pass the others around individually, and helps 
provide clarification for its use, but it does not preclude you from passing 
others around, when needed. A good example is the ZK-related utility code... it 
doesn't need credentials or configuration... just instanceId. For any 
thrift-RPC, the full context is usually needed. It's an improvement over the 
existing, but by no means is a perfect solution that forces us to use these 
things in a particular way. My hope is that it will be a stepping stone for 
future improvements. For example... a big area of confusion when debugging this 
change was the AccumuloReplicaSystem, which runs in the TabletServer. It has a 
TabletServer context, but it also has a ClientContext to talk to a remote peer, 
constructed from the current server's configuration. What this means is that 
the AccumuloReplicaSystem is limited to using the current server's SSL 
parameters to talk to the peer, because there is no way to configure the remote
 's SSL params (currently). That wasn't necessarily obvious from before this 
change. Now, it is clear that there are two distinct contexts.

That clarity is innate, though, because the context creates more readable code 
(IMO). I'm not arguing against more developer oriented documentation... but I 
do think that some design decisions can be helpful by their nature, by the 
impact it has on the code, and I'd hate to strictly prescribe how these things 
should be used in future.

In any case, I think this is a first step... I'm sure there's lots of 
opportunities for redesign, and better documentation for internals.


- Christopher


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28312/#review62775
---


On Nov. 21, 2014, 9:21 p.m., Christopher Tubbs wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28312/
> ---
> 
> (Updated Nov. 21, 2014, 9:21 p.m.)
> 
> 
> Review request for accumulo and Mike Drob.
> 
> 
> Bugs: ACCUMULO-3199 and ACCUMULO-3252
> https://issues.apache.org/jira/browse/ACCUMULO-3199
> https://issues.apache.org/jira/browse/ACCUMULO-3252
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> This patch introduces a new ClientContext object that contains Credentials,
> Instance, and Configuration provided from the client API. This new object is
> passed around internally in place of the previous three objects. An
> AccumuloServerContext is also introduced, which extends the ClientContext.
> Together, these objects ensure the proper configuration, credentials, and
> everything else needed to communicate with other system components are
> available to any RPC-related code.
> 
> These new object types also reduce the need to create multiple references to
> commonly used internal objects (such as HdfsZooInstance and
> SystemCredentials), and avoids storing information in static fields. As a
> side-effect, this should allow for better testing with mocked components.
> 
> This fixes ACCUMULO-3252, and may lay some groundwork for ACCUMULO-2589.
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java a6a5e81 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 
> 4e11a14 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/BatchWriterImpl.java 
> bd76a50 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ClientConfigurationHelper.java
>  603172f 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ClientContext.java 
> PRE-CREATION 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
>  cb0b90e 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 
> 2d077d9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/InstanceOperationsImpl.java
>  9848612 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 
> 53f178a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/MultiTableBatchWriterImpl.java
>  b09582a 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/NamespaceOperationsImpl.java
>  f756ef1 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
>  901128b 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
>  e45bcc9 
>   
> core/src/main/java/org/apache/accumulo/core/client/impl/RootTabletLocator.java
>  97d476b 
>

Re: Review Request 27829: ACCUMULO-1085

[keith]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27829/#review62866
---



server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java


try?


- kturner


On Nov. 24, 2014, 8:33 p.m., Eric Newton wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27829/
> ---
> 
> (Updated Nov. 24, 2014, 8:33 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-1085
> https://issues.apache.org/jira/browse/ACCUMULO-1085
> 
> 
> Repository: accumulo
> 
> 
> Description
> ---
> 
> Allow for multiple threads to perform assignment, but use only a single 
> thread for recovery playback
> 
> 
> Diffs
> -
> 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java 1195668 
>   server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 
> 93161ee 
>   
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java
>  ba86522 
>   test/src/test/java/org/apache/accumulo/test/AssignmentThreadsIT.java 
> PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/VerifySerialRecoveryIT.java 
> PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/functional/BulkFileIT.java 
> 475d5cf 
>   
> test/src/test/java/org/apache/accumulo/test/functional/FunctionalTestUtils.java
>  1246efe 
> 
> Diff: https://reviews.apache.org/r/27829/diff/
> 
> 
> Testing
> ---
> 
> Added an IT that verifies that tablet loading goes faster with more threads.
> 
> 
> Thanks,
> 
> Eric Newton
> 
>

Re: Review Request 28214: Decouple MiniAccumuloCluster from integration tests and introduce StandaloneAccumuloCluster

[Josh Elser]

> On Nov. 19, 2014, 5 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/Accumulo3047IT.java, line 77
> > 
> >
> > 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.

Will do in follow-on.


> On Nov. 19, 2014, 5 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/harness/conf/AccumuloStandaloneClusterConfiguration.java,
> >  line 32
> > 
> >
> > Will need to add some more pieces here to run against an instance with 
> > SSL enabled.

Will do in follow-on.


> On Nov. 19, 2014, 5 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/harness/AccumuloClusterIT.java, line 
> > 197
> > 
> >
> > 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.

Can address if better approach comes up later.


> On Nov. 19, 2014, 5 p.m., Josh Elser wrote:
> > minicluster/src/main/java/org/apache/accumulo/cluster/standalone/StandaloneClusterControl.java,
> >  line 290
> > 
> >
> > 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?

Will address when it becomes an issue.


> On Nov. 19, 2014, 5 p.m., Josh Elser wrote:
> > minicluster/src/main/java/org/apache/accumulo/cluster/ClusterControl.java, 
> > line 46
> > 
> >
> > 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.

Can remove later if desired, not intended for client use anyways.


> On Nov. 19, 2014, 5 p.m., Josh Elser wrote:
> > minicluster/src/main/java/org/apache/accumulo/cluster/ClusterControl.java, 
> > line 31
> > 
> >
> > Not sure if it's better to accept `String[] args` or `String... args`. 
> > MiniAccumuloClusterImpl's `exec` method did accept the varargs variant.

6 of one, half-dozen of another.


> On Nov. 19, 2014, 5 p.m., Josh Elser wrote:
> > test/src/test/java/org/apache/accumulo/test/functional/RestartIT.java, line 
> > 122
> > 
> >
> > 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?

I haven't hit a test that isn't satisfied by getting the return code and stdout 
or doing the above. Most aren't really forking off anyways.


- Josh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28214/#review62156
---


On Nov. 21, 2014, 8:40 p.m., Josh Elser wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28214/
> ---
> 
> (Updated Nov. 21, 2014, 8:40 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 StandaloneAccumuloCluste

Re: Review Request 28214: Decouple MiniAccumuloCluster from integration tests and introduce StandaloneAccumuloCluster

[Josh Elser]

> On Nov. 21, 2014, 2 a.m., kturner wrote:
> > minicluster/src/main/java/org/apache/accumulo/cluster/RemoteShell.java, 
> > line 34
> > 
> >
> > Took a quick look at ShellCommandExecutor, its marked unstable and 
> > private to hadoop using annotations.
> 
> Josh Elser wrote:
> Ah, I didn't take a look at that. RemoteShell was lifted from HBase, 
> actually (at least the general premise and some code).

Since this is internal to the user writing the test, I think I'm ok for the 
time being. If this turns out to be a major pain point, we can fork and make 
our own copy of ShellCommandExecutor.


- Josh


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28214/#review62476
---


On Nov. 21, 2014, 8:40 p.m., Josh Elser wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28214/
> ---
> 
> (Updated Nov. 21, 2014, 8:40 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 
>   server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java 
> 55548e3 
>   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/

Re: Review Request 28214: Decouple MiniAccumuloCluster from integration tests and introduce StandaloneAccumuloCluster

[Josh Elser]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28214/
---

(Updated Nov. 24, 2014, 11:23 p.m.)


Review request for accumulo.


Changes
---

Updated to what I believe will be the final changeset here. Fixing little 
things here and there are just adding up way too much -- things are pretty 
stable as long as I don't keep mucking with new tests. Running a suite of tests 
to make sure I didn't break anything new in the default `mvn verify`. Will 
definitely add some documentation before committing.


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 (updated)
-

  minicluster/pom.xml 5ad531f 
  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 
  server/gc/src/main/java/org/apache/accumulo/gc/SimpleGarbageCollector.java 
55548e3 
  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/StandaloneAccumuloClusterConfiguration.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