> On June 4, 2014, 8: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)
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. > On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java, > > lines 56-70 > > <https://reviews.apache.org/r/22003/diff/1/?file=598217#file598217line56> > > > > Are there other terms that are better than "drain"? "sync", for > > example, is a bit more descriptive, and has standard analogies in > > filesystems. > > Josh Elser wrote: > I'd be worried of implying that naming the method "sync" might imply that > it's actually doing something. I liked drain because it's more descriptive > that it's just waiting (e.g. for water to drain in a sink). It's not doing > anything active to *make* the replication happen. I can likely make better > javadoc here -- I had been waffling with how these methods should work for a > while, and they didn't get as much love as they should've once I finally > accepted them. > > Christopher Tubbs wrote: > I like the drain analogy, but from an API perspective, the user doesn't > really need to know here whether the implementation flushes queued > replication tasks or just waits for existing tasks to complete, or some other > implementation variant (and that could actually change over time), and the > semantics appear to be strikingly similar to coreutils' /bin/sync > > If all you want to provide is a wait loop, perhaps another option is to > just provide an "boolean isFullyReplicated();" method or similar so users can > write their own wait code. Alternatively, something very explicit might work > rather than an analogy, like "waitForFullReplication()" or similar. > > With good javadocs, drain works, but I think it could be more clear. > > (Also, the overloaded method with the additional files parameter needs to > be explained a bit more.) I improved the documentation on these methods already. I'm apt to let this ride for now and see if it's still misleading or confusing. I can add the isFullyReplicated method as I can see the merits of having that method as well and it's practically written for me already with the implementation of drain. > On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/conf/Property.java, line 433 > > <https://reviews.apache.org/r/22003/diff/1/?file=598231#file598231line433> > > > > Target or targets? enum and key should match in number. > > Josh Elser wrote: > I did it this way because it is a prefix. and thus there would be many > "table.replication.target." prefixes, but each key-value is singular and > represents a single target. Do you still think this is confusing? > > Christopher Tubbs wrote: > By convention, all our other property prefixes look like: > <SINGULAR_THING_THAT_IS_PREFIXED>_PREFIX > > The reason for this is because it doesn't prefix the set. Rather, it > prefixes each singular element of the set. Further, the enum and the key for > the enum are really identifiers for the same thing (in a slightly different > form), so it's very confusing for them to have different number. > Grammatically speaking, I think the convention is better. Ok, I'll remove the plural from the property key. > On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/data/ConditionalMutation.java, > > line 108 > > <https://reviews.apache.org/r/22003/diff/1/?file=598232#file598232line108> > > > > Maybe in a future version. Would be nice. > > Josh Elser wrote: > It's a possibility -- I don't know how to make that work yet. We can make > a ticket if you feel it would be prudent :) > > Christopher Tubbs wrote: > A ticket would be nice, for future work. ACCUMULO-2860 > On June 4, 2014, 8: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. 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. - Josh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22003/#review44749 ----------------------------------------------------------- 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 > >
