You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@accumulo.apache.org by Bill Havanki <bh...@clouderagovt.com> on 2014/06/03 20:11:33 UTC

Re: Review Request 22003: Replication: Client-facing changes

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



core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java
<https://reviews.apache.org/r/22003/#comment79059>

    The usual convention is to use third-person present tense instead of imperative, e.g., "defines" instead of "define". See http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide.
    
    Also, I think javadoc should have a first sentence that ends with a period / some other mark.



core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java
<https://reviews.apache.org/r/22003/#comment79060>

    Javadoc needs update.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79062>

    If this really needs to be a utility class, it needs a private constructor to avoid instantiation. But, I would *so much rather* it be a regular class to allow mocking / testing / dependency injection.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79064>

    Any reason to avoid checkNotNull here?



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79065>

    UtilWaitThread.sleep eats interrupts, so I don't like it. :) Can this do a regular sleep and handle interruption gracefully, perhaps by just throwing an AccumuloException?
    
    (I won't comment on any other uses of the method.)



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79066>

    Log the exception too



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79067>

    Avoid throwing an ordinary RuntimeException



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79068>

    The retry mechanism isn't in this method, so it shouldn't log that here.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79071>

    Consider adding an overall timeout and/or maximum number of attempts.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79070>

    This catches RuntimeExceptions too. Isn't there some awesome Java 7 way to do this? Can't remember.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java
<https://reviews.apache.org/r/22003/#comment79072>

    Consider adding an overall timeout and/or maximum number of attempts.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
<https://reviews.apache.org/r/22003/#comment79074>

    Make these fields final. Null check too?



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
<https://reviews.apache.org/r/22003/#comment79077>

    Looks like we could use an asynchronous / callback mechanism for this sort of thing.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
<https://reviews.apache.org/r/22003/#comment79079>

    Make the number of query threads configurable.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
<https://reviews.apache.org/r/22003/#comment79081>

    The singleton set here could be computed once outside the loop.



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
<https://reviews.apache.org/r/22003/#comment79082>

    Possible / beneficial to use the same batch scanner for each loop iteration?



core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java
<https://reviews.apache.org/r/22003/#comment79083>

    Refactor this with the same section in drain().



core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java
<https://reviews.apache.org/r/22003/#comment79084>

    Could adding a table ID to mock tables be a separate, smaller commit / ticket?



core/src/main/java/org/apache/accumulo/core/client/replication/PeerExistsException.java
<https://reviews.apache.org/r/22003/#comment79085>

    Add a constructor for (peer, message, cause)



core/src/main/java/org/apache/accumulo/core/client/replication/PeerNotFoundException.java
<https://reviews.apache.org/r/22003/#comment79086>

    Add a constructor for (peer, message, cause)



core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java
<https://reviews.apache.org/r/22003/#comment79087>

    Missing @param for helper



core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java
<https://reviews.apache.org/r/22003/#comment79088>

    Would it be useful to define an over-arching sort of ReplicationException for this?



core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java
<https://reviews.apache.org/r/22003/#comment79089>

    sp: quorum



core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java
<https://reviews.apache.org/r/22003/#comment79090>

    Needs a private constructor



core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java
<https://reviews.apache.org/r/22003/#comment79092>

    Just check clz? Or use o instanceof ReplicaSystem



core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java
<https://reviews.apache.org/r/22003/#comment79091>

    ew, throwing RuntimeException



core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java
<https://reviews.apache.org/r/22003/#comment79093>

    This could return a value which the get() method will pull an empty-string configuration from. Is that OK?


- Bill Havanki


On May 28, 2014, 10:52 p.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22003/
> -----------------------------------------------------------
> 
> (Updated May 28, 2014, 10:52 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-378
>     https://issues.apache.org/jira/browse/ACCUMULO-378
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> "Public-facing" changes. Thrift IDLs, protobufs, client API additions, monitor additions.
> 
> 
> Diffs
> -----
> 
>   core/pom.xml 2c3f648 
>   core/src/main/java/org/apache/accumulo/core/Constants.java 7d602bb 
>   core/src/main/java/org/apache/accumulo/core/client/Connector.java 4a2acff 
>   core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 0df35f6 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 1fa8f12 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 2c26ecc 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java 996198c 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java cb50761 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java a44a027 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerExistsException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerNotFoundException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicationTable.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java 1200fd1 
>   core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java e43968c 
>   core/src/main/java/org/apache/accumulo/core/data/Mutation.java 42fa143 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/MultiScanResult.java f0b8a87 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/ScanResult.java 6472587 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java 3f9b3f7 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TMutation.java f698892 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/UpdateErrors.java 59ce5cb 
>   core/src/main/java/org/apache/accumulo/core/iterators/Combiner.java ceb4411 
>   core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java 06eae23 
>   core/src/main/java/org/apache/accumulo/core/protobuf/ProtobufUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/PrintReplicationRecords.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicaSystemHelper.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationSchema.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationTarget.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusFormatter.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/KeyValues.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinator.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationServicer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/WalEdits.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/schema/Section.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/tabletserver/thrift/ReplicationFailedException.java PRE-CREATION 
>   core/src/main/protobuf/replication.proto PRE-CREATION 
>   core/src/main/scripts/generate-protobuf.sh PRE-CREATION 
>   core/src/main/scripts/generate-thrift.sh 5a5d69f 
>   core/src/main/thrift/data.thrift ae6f439 
>   core/src/main/thrift/replication.thrift PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationOperationsImplTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationSchemaTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationTargetTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/StatusUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/proto/StatusTest.java PRE-CREATION 
>   docs/src/main/asciidoc/accumulo_user_manual.asciidoc fec40ca 
>   docs/src/main/asciidoc/chapters/replication.txt PRE-CREATION 
>   docs/src/main/resources/design/ACCUMULO-378-design.mdtext PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.gv PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.png PRE-CREATION 
>   minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 3258991 
>   server/monitor/pom.xml 411812c 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java e6617d0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java 86a84b9 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/ReplicationServlet.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22003/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Josh Elser
> 
>

Re: Review Request 22003: Replication: Client-facing changes

Posted by Josh Elser <jo...@gmail.com>.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 46
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line46>
> >
> >     If this really needs to be a utility class, it needs a private constructor to avoid instantiation. But, I would *so much rather* it be a regular class to allow mocking / testing / dependency injection.
> 
> Josh Elser wrote:
>     The "cruft" of this class was inherited by copying it from MasterClient. I can understand where you're coming from, but I will say that most of the code in here isn't the "critical" thing to directly test. It is covered by the other tests which use this to actually connect to a remote service.
>     
>     I could convert it to a regular class with instance methods, or some factory class which returns instances of these connections, but honestly I don't see that buying us much anything in respect to code coverage or practical tests.
> 
> Bill Havanki wrote:
>     That's fine with me. Keeping it consistent with other similar classes is a good goal. This is something we can re-address later if it does become problematic, but if you don't see it being useful to do now, that's good enough for me.
> 
> Josh Elser wrote:
>     I'll open up a general ticket to revisit both of these classes and see what we can extract into a more testable, common base.

Created ACCUMULO-2867


- Josh


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


On May 29, 2014, 2:52 a.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22003/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 2:52 a.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-378
>     https://issues.apache.org/jira/browse/ACCUMULO-378
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> "Public-facing" changes. Thrift IDLs, protobufs, client API additions, monitor additions.
> 
> 
> Diffs
> -----
> 
>   core/pom.xml 2c3f648 
>   core/src/main/java/org/apache/accumulo/core/Constants.java 7d602bb 
>   core/src/main/java/org/apache/accumulo/core/client/Connector.java 4a2acff 
>   core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 0df35f6 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 1fa8f12 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 2c26ecc 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java 996198c 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java cb50761 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java a44a027 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerExistsException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerNotFoundException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicationTable.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java 1200fd1 
>   core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java e43968c 
>   core/src/main/java/org/apache/accumulo/core/data/Mutation.java 42fa143 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/MultiScanResult.java f0b8a87 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/ScanResult.java 6472587 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java 3f9b3f7 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TMutation.java f698892 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/UpdateErrors.java 59ce5cb 
>   core/src/main/java/org/apache/accumulo/core/iterators/Combiner.java ceb4411 
>   core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java 06eae23 
>   core/src/main/java/org/apache/accumulo/core/protobuf/ProtobufUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/PrintReplicationRecords.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicaSystemHelper.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationSchema.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationTarget.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusFormatter.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/KeyValues.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinator.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationServicer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/WalEdits.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/schema/Section.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/tabletserver/thrift/ReplicationFailedException.java PRE-CREATION 
>   core/src/main/protobuf/replication.proto PRE-CREATION 
>   core/src/main/scripts/generate-protobuf.sh PRE-CREATION 
>   core/src/main/scripts/generate-thrift.sh 5a5d69f 
>   core/src/main/thrift/data.thrift ae6f439 
>   core/src/main/thrift/replication.thrift PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationOperationsImplTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationSchemaTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationTargetTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/StatusUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/proto/StatusTest.java PRE-CREATION 
>   docs/src/main/asciidoc/accumulo_user_manual.asciidoc fec40ca 
>   docs/src/main/asciidoc/chapters/replication.txt PRE-CREATION 
>   docs/src/main/resources/design/ACCUMULO-378-design.mdtext PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.gv PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.png PRE-CREATION 
>   minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 3258991 
>   server/monitor/pom.xml 411812c 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java e6617d0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java 86a84b9 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/ReplicationServlet.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22003/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Josh Elser
> 
>

Re: Review Request 22003: Replication: Client-facing changes

Posted by Josh Elser <jo...@gmail.com>.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java, line 35
> > <https://reviews.apache.org/r/22003/diff/1/?file=598217#file598217line35>
> >
> >     The usual convention is to use third-person present tense instead of imperative, e.g., "defines" instead of "define". See http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide.
> >     
> >     Also, I think javadoc should have a first sentence that ends with a period / some other mark.

Fine by me!


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 55
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line55>
> >
> >     Any reason to avoid checkNotNull here?

Nah, was just a copy-paste I didn't fix from MasterClient.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 46
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line46>
> >
> >     If this really needs to be a utility class, it needs a private constructor to avoid instantiation. But, I would *so much rather* it be a regular class to allow mocking / testing / dependency injection.

The "cruft" of this class was inherited by copying it from MasterClient. I can understand where you're coming from, but I will say that most of the code in here isn't the "critical" thing to directly test. It is covered by the other tests which use this to actually connect to a remote service.

I could convert it to a regular class with instance methods, or some factory class which returns instances of these connections, but honestly I don't see that buying us much anything in respect to code coverage or practical tests.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 94
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line94>
> >
> >     Log the exception too

This was actually intentional to avoid spam (e.g. a peer is offline). I think this might be less of a worry now that the caller does the sleep with a backoff.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 111
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line111>
> >
> >     Avoid throwing an ordinary RuntimeException

Not as "deadly" when talking to a peer's Master (as opposed to our Master). Just removing it completely, and letting it cycle normally.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 113
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line113>
> >
> >     The retry mechanism isn't in this method, so it shouldn't log that here.

More copy-paste. Left real description in place, lifted retry message to caller.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java, line 152
> > <https://reviews.apache.org/r/22003/diff/1/?file=598221#file598221line152>
> >
> >     Make the number of query threads configurable.

4 threads should be fine as a default, but created ACCUMULO-2856 to make these thread pool sizes configurable if something comes up that I didn't think of.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java, line 158
> > <https://reviews.apache.org/r/22003/diff/1/?file=598221#file598221line158>
> >
> >     Possible / beneficial to use the same batch scanner for each loop iteration?

BatchScanners can't be re-used.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java, line 95
> > <https://reviews.apache.org/r/22003/diff/1/?file=598224#file598224line95>
> >
> >     Could adding a table ID to mock tables be a separate, smaller commit / ticket?

Ugh, I forgot about this one. I don't remember what I was actually invoking that created the underlying exception. I think it was TableOperations.tableIdMap not working with MockInstance. I can lift it out, but there is more included that just these changes (MockTableOperations, at least). Filed ACCUMULO-2857


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java, line 36
> > <https://reviews.apache.org/r/22003/diff/1/?file=598228#file598228line36>
> >
> >     Would it be useful to define an over-arching sort of ReplicationException for this?

I don't think so, because then we would have to bake the logic of what exactly to do when that exception is seen into the TabletServer. This forces the implementation to know what its doing and how to recover from Exceptions, rather than let that bubble up into the core Accumulo logic.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, line 50
> > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line50>
> >
> >     Just check clz? Or use o instanceof ReplicaSystem

Is there actually a difference between what's in the code and what `instanceof ReplicaSystem` would actually do? Could I have a class that implements ReplicaSystem that isn't an instanceof it? I don't know, tbh.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, line 59
> > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line59>
> >
> >     ew, throwing RuntimeException

Changed to IllegalArgumentException (assuming your issue was with the untyped RTE)


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, line 75
> > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line75>
> >
> >     This could return a value which the get() method will pull an empty-string configuration from. Is that OK?

This isn't pulling data from a Configuration object, merely providing "serialization" that would go into the value of a Property. I think this is fine as-is.


- Josh


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


On May 29, 2014, 2:52 a.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22003/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 2:52 a.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-378
>     https://issues.apache.org/jira/browse/ACCUMULO-378
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> "Public-facing" changes. Thrift IDLs, protobufs, client API additions, monitor additions.
> 
> 
> Diffs
> -----
> 
>   core/pom.xml 2c3f648 
>   core/src/main/java/org/apache/accumulo/core/Constants.java 7d602bb 
>   core/src/main/java/org/apache/accumulo/core/client/Connector.java 4a2acff 
>   core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 0df35f6 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 1fa8f12 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 2c26ecc 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java 996198c 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java cb50761 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java a44a027 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerExistsException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerNotFoundException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicationTable.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java 1200fd1 
>   core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java e43968c 
>   core/src/main/java/org/apache/accumulo/core/data/Mutation.java 42fa143 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/MultiScanResult.java f0b8a87 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/ScanResult.java 6472587 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java 3f9b3f7 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TMutation.java f698892 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/UpdateErrors.java 59ce5cb 
>   core/src/main/java/org/apache/accumulo/core/iterators/Combiner.java ceb4411 
>   core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java 06eae23 
>   core/src/main/java/org/apache/accumulo/core/protobuf/ProtobufUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/PrintReplicationRecords.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicaSystemHelper.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationSchema.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationTarget.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusFormatter.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/KeyValues.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinator.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationServicer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/WalEdits.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/schema/Section.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/tabletserver/thrift/ReplicationFailedException.java PRE-CREATION 
>   core/src/main/protobuf/replication.proto PRE-CREATION 
>   core/src/main/scripts/generate-protobuf.sh PRE-CREATION 
>   core/src/main/scripts/generate-thrift.sh 5a5d69f 
>   core/src/main/thrift/data.thrift ae6f439 
>   core/src/main/thrift/replication.thrift PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationOperationsImplTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationSchemaTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationTargetTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/StatusUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/proto/StatusTest.java PRE-CREATION 
>   docs/src/main/asciidoc/accumulo_user_manual.asciidoc fec40ca 
>   docs/src/main/asciidoc/chapters/replication.txt PRE-CREATION 
>   docs/src/main/resources/design/ACCUMULO-378-design.mdtext PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.gv PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.png PRE-CREATION 
>   minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 3258991 
>   server/monitor/pom.xml 411812c 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java e6617d0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java 86a84b9 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/ReplicationServlet.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22003/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Josh Elser
> 
>

Re: Review Request 22003: Replication: Client-facing changes

Posted by Bill Havanki <bh...@clouderagovt.com>.


> On June 3, 2014, 2:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 46
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line46>
> >
> >     If this really needs to be a utility class, it needs a private constructor to avoid instantiation. But, I would *so much rather* it be a regular class to allow mocking / testing / dependency injection.
> 
> Josh Elser wrote:
>     The "cruft" of this class was inherited by copying it from MasterClient. I can understand where you're coming from, but I will say that most of the code in here isn't the "critical" thing to directly test. It is covered by the other tests which use this to actually connect to a remote service.
>     
>     I could convert it to a regular class with instance methods, or some factory class which returns instances of these connections, but honestly I don't see that buying us much anything in respect to code coverage or practical tests.

That's fine with me. Keeping it consistent with other similar classes is a good goal. This is something we can re-address later if it does become problematic, but if you don't see it being useful to do now, that's good enough for me.


> On June 3, 2014, 2:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 94
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line94>
> >
> >     Log the exception too
> 
> Josh Elser wrote:
>     This was actually intentional to avoid spam (e.g. a peer is offline). I think this might be less of a worry now that the caller does the sleep with a backoff.

OK


> On June 3, 2014, 2:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, line 59
> > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line59>
> >
> >     ew, throwing RuntimeException
> 
> Josh Elser wrote:
>     Changed to IllegalArgumentException (assuming your issue was with the untyped RTE)

It was, thanks :)


> On June 3, 2014, 2:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java, line 36
> > <https://reviews.apache.org/r/22003/diff/1/?file=598228#file598228line36>
> >
> >     Would it be useful to define an over-arching sort of ReplicationException for this?
> 
> Josh Elser wrote:
>     I don't think so, because then we would have to bake the logic of what exactly to do when that exception is seen into the TabletServer. This forces the implementation to know what its doing and how to recover from Exceptions, rather than let that bubble up into the core Accumulo logic.

OK, that's a good design constraint. Along the same lines, if some odd thing happens that can't be handled within the replication logic, tossing a RuntimeException would be appropriate.


> On June 3, 2014, 2:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, line 50
> > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line50>
> >
> >     Just check clz? Or use o instanceof ReplicaSystem
> 
> Josh Elser wrote:
>     Is there actually a difference between what's in the code and what `instanceof ReplicaSystem` would actually do? Could I have a class that implements ReplicaSystem that isn't an instanceof it? I don't know, tbh.

Sorry, should have explained my thought process more. You could just do isAssignableFrom(clz) right away, without making an instance, and detect the case where clz isn't ReplicaSystem.class or a subclass. Then you wouldn't have to call Class.newInstance on that class type when it ends up useless anyway.


> On June 3, 2014, 2:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, line 75
> > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line75>
> >
> >     This could return a value which the get() method will pull an empty-string configuration from. Is that OK?
> 
> Josh Elser wrote:
>     This isn't pulling data from a Configuration object, merely providing "serialization" that would go into the value of a Property. I think this is fine as-is.

OK


- Bill


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


On May 28, 2014, 10:52 p.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22003/
> -----------------------------------------------------------
> 
> (Updated May 28, 2014, 10:52 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-378
>     https://issues.apache.org/jira/browse/ACCUMULO-378
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> "Public-facing" changes. Thrift IDLs, protobufs, client API additions, monitor additions.
> 
> 
> Diffs
> -----
> 
>   core/pom.xml 2c3f648 
>   core/src/main/java/org/apache/accumulo/core/Constants.java 7d602bb 
>   core/src/main/java/org/apache/accumulo/core/client/Connector.java 4a2acff 
>   core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 0df35f6 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 1fa8f12 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 2c26ecc 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java 996198c 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java cb50761 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java a44a027 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerExistsException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerNotFoundException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicationTable.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java 1200fd1 
>   core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java e43968c 
>   core/src/main/java/org/apache/accumulo/core/data/Mutation.java 42fa143 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/MultiScanResult.java f0b8a87 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/ScanResult.java 6472587 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java 3f9b3f7 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TMutation.java f698892 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/UpdateErrors.java 59ce5cb 
>   core/src/main/java/org/apache/accumulo/core/iterators/Combiner.java ceb4411 
>   core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java 06eae23 
>   core/src/main/java/org/apache/accumulo/core/protobuf/ProtobufUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/PrintReplicationRecords.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicaSystemHelper.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationSchema.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationTarget.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusFormatter.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/KeyValues.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinator.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationServicer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/WalEdits.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/schema/Section.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/tabletserver/thrift/ReplicationFailedException.java PRE-CREATION 
>   core/src/main/protobuf/replication.proto PRE-CREATION 
>   core/src/main/scripts/generate-protobuf.sh PRE-CREATION 
>   core/src/main/scripts/generate-thrift.sh 5a5d69f 
>   core/src/main/thrift/data.thrift ae6f439 
>   core/src/main/thrift/replication.thrift PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationOperationsImplTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationSchemaTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationTargetTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/StatusUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/proto/StatusTest.java PRE-CREATION 
>   docs/src/main/asciidoc/accumulo_user_manual.asciidoc fec40ca 
>   docs/src/main/asciidoc/chapters/replication.txt PRE-CREATION 
>   docs/src/main/resources/design/ACCUMULO-378-design.mdtext PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.gv PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.png PRE-CREATION 
>   minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 3258991 
>   server/monitor/pom.xml 411812c 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java e6617d0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java 86a84b9 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/ReplicationServlet.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22003/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Josh Elser
> 
>

Re: Review Request 22003: Replication: Client-facing changes

Posted by Josh Elser <jo...@gmail.com>.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, line 46
> > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line46>
> >
> >     If this really needs to be a utility class, it needs a private constructor to avoid instantiation. But, I would *so much rather* it be a regular class to allow mocking / testing / dependency injection.
> 
> Josh Elser wrote:
>     The "cruft" of this class was inherited by copying it from MasterClient. I can understand where you're coming from, but I will say that most of the code in here isn't the "critical" thing to directly test. It is covered by the other tests which use this to actually connect to a remote service.
>     
>     I could convert it to a regular class with instance methods, or some factory class which returns instances of these connections, but honestly I don't see that buying us much anything in respect to code coverage or practical tests.
> 
> Bill Havanki wrote:
>     That's fine with me. Keeping it consistent with other similar classes is a good goal. This is something we can re-address later if it does become problematic, but if you don't see it being useful to do now, that's good enough for me.

I'll open up a general ticket to revisit both of these classes and see what we can extract into a more testable, common base.


> On June 3, 2014, 6:11 p.m., Bill Havanki wrote:
> > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, line 50
> > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line50>
> >
> >     Just check clz? Or use o instanceof ReplicaSystem
> 
> Josh Elser wrote:
>     Is there actually a difference between what's in the code and what `instanceof ReplicaSystem` would actually do? Could I have a class that implements ReplicaSystem that isn't an instanceof it? I don't know, tbh.
> 
> Bill Havanki wrote:
>     Sorry, should have explained my thought process more. You could just do isAssignableFrom(clz) right away, without making an instance, and detect the case where clz isn't ReplicaSystem.class or a subclass. Then you wouldn't have to call Class.newInstance on that class type when it ends up useless anyway.

Ahhh, ok, I got ya.


- Josh


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


On May 29, 2014, 2:52 a.m., Josh Elser wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/22003/
> -----------------------------------------------------------
> 
> (Updated May 29, 2014, 2:52 a.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-378
>     https://issues.apache.org/jira/browse/ACCUMULO-378
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> "Public-facing" changes. Thrift IDLs, protobufs, client API additions, monitor additions.
> 
> 
> Diffs
> -----
> 
>   core/pom.xml 2c3f648 
>   core/src/main/java/org/apache/accumulo/core/Constants.java 7d602bb 
>   core/src/main/java/org/apache/accumulo/core/client/Connector.java 4a2acff 
>   core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConnectorImpl.java 0df35f6 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 1fa8f12 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 2c26ecc 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockConnector.java 996198c 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java cb50761 
>   core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java a44a027 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerExistsException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/PeerNotFoundException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/client/replication/ReplicationTable.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/conf/Property.java 1200fd1 
>   core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java e43968c 
>   core/src/main/java/org/apache/accumulo/core/data/Mutation.java 42fa143 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/MultiScanResult.java f0b8a87 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/ScanResult.java 6472587 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java 3f9b3f7 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/TMutation.java f698892 
>   core/src/main/java/org/apache/accumulo/core/data/thrift/UpdateErrors.java 59ce5cb 
>   core/src/main/java/org/apache/accumulo/core/iterators/Combiner.java ceb4411 
>   core/src/main/java/org/apache/accumulo/core/metadata/schema/MetadataSchema.java 06eae23 
>   core/src/main/java/org/apache/accumulo/core/protobuf/ProtobufUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/AccumuloReplicationReplayer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/PrintReplicationRecords.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicaSystemHelper.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationSchema.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/ReplicationTarget.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusFormatter.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/StatusUtil.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/proto/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/KeyValues.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/RemoteReplicationException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/Replication.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinator.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorErrorCode.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationCoordinatorException.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/ReplicationServicer.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/replication/thrift/WalEdits.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/schema/Section.java PRE-CREATION 
>   core/src/main/java/org/apache/accumulo/core/tabletserver/thrift/ReplicationFailedException.java PRE-CREATION 
>   core/src/main/protobuf/replication.proto PRE-CREATION 
>   core/src/main/scripts/generate-protobuf.sh PRE-CREATION 
>   core/src/main/scripts/generate-thrift.sh 5a5d69f 
>   core/src/main/thrift/data.thrift ae6f439 
>   core/src/main/thrift/replication.thrift PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationConfigurationUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationOperationsImplTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationSchemaTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/ReplicationTargetTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/StatusUtilTest.java PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/replication/proto/StatusTest.java PRE-CREATION 
>   docs/src/main/asciidoc/accumulo_user_manual.asciidoc fec40ca 
>   docs/src/main/asciidoc/chapters/replication.txt PRE-CREATION 
>   docs/src/main/resources/design/ACCUMULO-378-design.mdtext PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.gv PRE-CREATION 
>   docs/src/main/resources/state/replicationstatus.png PRE-CREATION 
>   minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 3258991 
>   server/monitor/pom.xml 411812c 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/Monitor.java e6617d0 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/BasicServlet.java 86a84b9 
>   server/monitor/src/main/java/org/apache/accumulo/monitor/servlets/ReplicationServlet.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/22003/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Josh Elser
> 
>