> On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java, > > line 35 > > <https://reviews.apache.org/r/22003/diff/1/?file=598217#file598217line35> > > > > The usual convention is to use third-person present tense instead of > > imperative, e.g., "defines" instead of "define". See > > http://www.oracle.com/technetwork/java/javase/documentation/index-137868.html#styleguide. > > > > Also, I think javadoc should have a first sentence that ends with a > > period / some other mark.
Fine by me! > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, > > line 55 > > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line55> > > > > Any reason to avoid checkNotNull here? Nah, was just a copy-paste I didn't fix from MasterClient. > 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. 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. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, > > line 94 > > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line94> > > > > Log the exception too This was actually intentional to avoid spam (e.g. a peer is offline). I think this might be less of a worry now that the caller does the sleep with a backoff. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, > > line 111 > > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line111> > > > > Avoid throwing an ordinary RuntimeException Not as "deadly" when talking to a peer's Master (as opposed to our Master). Just removing it completely, and letting it cycle normally. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationClient.java, > > line 113 > > <https://reviews.apache.org/r/22003/diff/1/?file=598220#file598220line113> > > > > The retry mechanism isn't in this method, so it shouldn't log that here. More copy-paste. Left real description in place, lifted retry message to caller. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java, > > line 152 > > <https://reviews.apache.org/r/22003/diff/1/?file=598221#file598221line152> > > > > Make the number of query threads configurable. 4 threads should be fine as a default, but created ACCUMULO-2856 to make these thread pool sizes configurable if something comes up that I didn't think of. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/ReplicationOperationsImpl.java, > > line 158 > > <https://reviews.apache.org/r/22003/diff/1/?file=598221#file598221line158> > > > > Possible / beneficial to use the same batch scanner for each loop > > iteration? BatchScanners can't be re-used. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java, > > line 95 > > <https://reviews.apache.org/r/22003/diff/1/?file=598224#file598224line95> > > > > Could adding a table ID to mock tables be a separate, smaller commit / > > ticket? Ugh, I forgot about this one. I don't remember what I was actually invoking that created the underlying exception. I think it was TableOperations.tableIdMap not working with MockInstance. I can lift it out, but there is more included that just these changes (MockTableOperations, at least). Filed ACCUMULO-2857 > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystem.java, > > line 36 > > <https://reviews.apache.org/r/22003/diff/1/?file=598228#file598228line36> > > > > Would it be useful to define an over-arching sort of > > ReplicationException for this? I don't think so, because then we would have to bake the logic of what exactly to do when that exception is seen into the TabletServer. This forces the implementation to know what its doing and how to recover from Exceptions, rather than let that bubble up into the core Accumulo logic. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, > > line 50 > > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line50> > > > > Just check clz? Or use o instanceof ReplicaSystem Is there actually a difference between what's in the code and what `instanceof ReplicaSystem` would actually do? Could I have a class that implements ReplicaSystem that isn't an instanceof it? I don't know, tbh. > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, > > line 59 > > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line59> > > > > ew, throwing RuntimeException Changed to IllegalArgumentException (assuming your issue was with the untyped RTE) > On June 3, 2014, 6:11 p.m., Bill Havanki wrote: > > core/src/main/java/org/apache/accumulo/core/client/replication/ReplicaSystemFactory.java, > > line 75 > > <https://reviews.apache.org/r/22003/diff/1/?file=598229#file598229line75> > > > > This could return a value which the get() method will pull an > > empty-string configuration from. Is that OK? This isn't pulling data from a Configuration object, merely providing "serialization" that would go into the value of a Property. I think this is fine as-is. - 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 > >
