accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 22003: Replication: Client-facing changes
Date Wed, 04 Jun 2014 20:59:55 GMT

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


This is a partial review. I didn't examine all the implementation stuff. One question I had
was whether or not this adds a new dependency on protocol buffers? If so, where is the pom.xml
change for that? Another patch?


core/src/main/java/org/apache/accumulo/core/Constants.java
<https://reviews.apache.org/r/22003/#comment79278>

    Please don't add new global constants if at all possible. Constants should be scoped to
the thing they are for. (eg. Replication.ZKPATH, Replication.WORKQUEUEPATH, etc.)



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

    What is the difference between a ReplicaSystem parameter and the class name of a ReplicaSystem?
Will the class name be instantiated on behalf of the user?
    
    Also, is String the best type for the class name? Or would Class<? extends ReplicaSystem>
be better?



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

    Are there other terms that are better than "drain"? "sync", for example, is a bit more
descriptive, and has standard analogies in filesystems.



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

    What does "referenced files" mean? Referenced how? This needs javadoc update to clarify,
and the answer may yield a better method name. At the very least, a getReferencedFile would
be helpful to identify it clearly as a getter without side-effects.



core/src/main/java/org/apache/accumulo/core/conf/Property.java
<https://reviews.apache.org/r/22003/#comment79283>

    Target or targets? enum and key should match in number.



core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java
<https://reviews.apache.org/r/22003/#comment79284>

    Maybe in a future version. Would be nice.



core/src/main/java/org/apache/accumulo/core/data/Mutation.java
<https://reviews.apache.org/r/22003/#comment79285>

    Please apply project formatter to strip trailing whitespace on blank lines. (commenting
here, but applies to entire patch set)



core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java
<https://reviews.apache.org/r/22003/#comment79287>

    Next time, please exclude generated thrift from review. :)



core/src/main/java/org/apache/accumulo/core/iterators/Combiner.java
<https://reviews.apache.org/r/22003/#comment79288>

    Are these necessary to add to the public API?


- Christopher Tubbs


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