accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 22003: Replication: Client-facing changes
Date Tue, 03 Jun 2014 18:11:33 GMT

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


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