> 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.
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) > On June 4, 2014, 4: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. 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.) > On June 4, 2014, 4: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? 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. > On June 4, 2014, 4: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 :) A ticket would be nice, for future work. > 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? 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. - 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 > >
