accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 22003: Replication: Client-facing changes
Date Fri, 06 Jun 2014 16:59:17 GMT


> On June 3, 2014, 2: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.
> 
> Josh Elser wrote:
>     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.

That's fine with me. Keeping it consistent with other similar classes is a good goal. This
is something we can re-address later if it does become problematic, but if you don't see it
being useful to do now, that's good enough for me.


> On June 3, 2014, 2: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
> 
> Josh Elser wrote:
>     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.

OK


> On June 3, 2014, 2: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
> 
> Josh Elser wrote:
>     Changed to IllegalArgumentException (assuming your issue was with the untyped RTE)

It was, thanks :)


> On June 3, 2014, 2: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?
> 
> Josh Elser wrote:
>     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.

OK, that's a good design constraint. Along the same lines, if some odd thing happens that
can't be handled within the replication logic, tossing a RuntimeException would be appropriate.


> On June 3, 2014, 2: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
> 
> Josh Elser wrote:
>     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.

Sorry, should have explained my thought process more. You could just do isAssignableFrom(clz)
right away, without making an instance, and detect the case where clz isn't ReplicaSystem.class
or a subclass. Then you wouldn't have to call Class.newInstance on that class type when it
ends up useless anyway.


> On June 3, 2014, 2: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?
> 
> Josh Elser wrote:
>     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.

OK


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/22003/#review44646
-----------------------------------------------------------


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


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