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