> On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote: > > This is a partial review. I didn't examine all the implementation stuff. > > One question I had was whether or not this adds a new dependency on > > protocol buffers? If so, where is the pom.xml change for that? Another > > patch?
Yes, I use protocol buffers. No, it does not require any dependencies. Like thrift, it just generates new Java files which are included, but there is no runtime library required to use them. They are self-contained. Hadoop has a maven plugin that could automate the re-generation of them when the definition needs to be updated, but I didn't bother trying to wire that up since, last I checked, it wasn't in central (and would be a bigger pain to force devs to install over just installing protoc) > 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? 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. > 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. 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. > On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java, > > line 77 > > <https://reviews.apache.org/r/22003/diff/1/?file=598217#file598217line77> > > > > What does "referenced files" mean? Referenced how? This needs javadoc > > update to clarify, and the answer may yield a better method name. At the > > very least, a getReferencedFile would be helpful to identify it clearly as > > a getter without side-effects. Noted -- same back-story about drain applies here. I can make better docs, maybe a better name too. > 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. 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 :) > On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/data/Mutation.java, line 919 > > <https://reviews.apache.org/r/22003/diff/1/?file=598233#file598233line919> > > > > Please apply project formatter to strip trailing whitespace on blank > > lines. (commenting here, but applies to entire patch set) Yes, I intended to reformat everything before I commit. It's a waste of time for me to incrementally re-apply the formatter after every commit I make (assuming I goof somewhere every time). > On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java, > > lines 617-646 > > <https://reviews.apache.org/r/22003/diff/1/?file=598236#file598236line617> > > > > Next time, please exclude generated thrift from review. :) Hahaha, psh. Sorry, I didn't think about excluding them when I was copy/pasting these diffs. > 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? 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? - 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 > >
