accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 22003: Replication: Client-facing changes
Date Wed, 04 Jun 2014 02:44:30 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message