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