> On June 4, 2014, 4:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java, > > lines 34-47 > > <https://reviews.apache.org/r/22003/diff/1/?file=598217#file598217line34> > > > > 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? > > Josh Elser wrote: > No difference in the implementation, just trying to be flexible in what > variants are available. Class<? extends ReplicaSystem> would also make sense > -- I'll add that. > > Christopher Tubbs wrote: > I think there would be a slight difference in implementation... because > you have to instantiate the ReplicaSystem if you're not passed an instance. > The javadoc should explain how that instantiation works (no-arg constructor > and init(...) method, or only no-arg constructor, or something else) > > Josh Elser wrote: > No, that is wrong. You should look at the implementation (it should be > included in this review). The TabletServer needs to know the class name of a > ReplicaSystem that it is to use to replicate data to a peer. This method does > no instantiation of anything. A public no-args constructor is required on the > ReplicaSystem, so I can add documentation there.
Ah, I see. It's just being passed as configuration. In that case, I think the method that accepts an instantiated ReplicaSystem object as a param is confusing. > On June 4, 2014, 4:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/iterators/Combiner.java, lines > > 56-57 > > <https://reviews.apache.org/r/22003/diff/1/?file=598239#file598239line56> > > > > Are these necessary to add to the public API? > > Josh Elser wrote: > Ah, I had to look, but the reason I did this is so that I could actually > test that the replication table had the correct configuration set. If I'm > doing it, I don't see why users might not also want to do the same. By having > it be a public variable, we're not advertising that the value itself is > fixed, just that the variable will be there? Is that still too much? > > Christopher Tubbs wrote: > The problem with that is that some consumers of the API will make > assumptions about how we use these fields internally (for example, to > serialize to a configuration). This has happened in the past with the > MapReduce configuration and constants in the Input/OutputFormats. > > Putting them in the public API is fine, I suppose, as long as we > understand that doing so implies that our internal usage of them will be > stable, and that may restrict us from restructuring the internals of Combiner > (for example, for configuration serialization) without breaking users > expectations. > > Scenario: > 1. User uses constant to manually generate per-table configuration for > their combiner. > 2. We change the combiner serialization such that it no longer has need > of this constant. > 3. User's generated per-table config is now broken and they can't figure > out why, because they are using a public Constant (and they don't realize > Combiner now ignores it). > > Keeping it protected reduces these kinds of risks by restricting problems > to only users who write their own Combiners. > > Granted, it's a small risk, and it's unlikely Combiner is going to change > its use of these constants much, if ever. I'm just thinking that if we don't > *need* to make it more visible, we should be careful about doing so. To > support a test, I suspect you could simply create an anonymous inner Combiner > to grab the value of the constant, but perhaps that's too clunky? > > I don't feel strongly about this... just trying to be cautious, based on > past experience with exposing Constants in the core Constants.java file and > in the MapReduce. > > Josh Elser wrote: > Sure, that's a valid risk; however, if these were ever changed, our tests > would also break (at least the test that I wrote which uses these constants). > Do you have a possible solution to the issue that I have where I can't > validate the configuration of a table that uses a Combiner? I guess I could > try to Mock out the code which creates and configures the replication table > and just expect that the already-public methods on Combiner get invoked. Well, you could use the clunky way I described to get the value of the constant from an anonymous subclass, in your test. But... it sounds like it's not really worth it. - Christopher ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22003/#review44749 ----------------------------------------------------------- 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 > >
