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 Thu, 05 Jun 2014 20:03:01 GMT


> 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?
> 
> 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.
> 
> Christopher Tubbs wrote:
>     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)
> 
> Josh Elser wrote:
>     No, that is wrong. You should look at the implementation (it should be included in
this review). The TabletServer needs to know the class name of a ReplicaSystem that it is
to use to replicate data to a peer. This method does no instantiation of anything. A public
no-args constructor is required on the ReplicaSystem, so I can add documentation there.
> 
> Christopher Tubbs wrote:
>     Ah, I see. It's just being passed as configuration. In that case, I think the method
that accepts an instantiated ReplicaSystem object as a param is confusing.

Yeah, that's valid. I probably already had an instance of the ReplicaSystem implementation
because I was writing a unit test. In practice, there's no reason for a client to instantiate
it.


> 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?
> 
> 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?
> 
> Christopher Tubbs wrote:
>     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.
> 
> Josh Elser wrote:
>     Sure, that's a valid risk; however, if these were ever changed, our tests would also
break (at least the test that I wrote which uses these constants). Do you have a possible
solution to the issue that I have where I can't validate the configuration of a table that
uses a Combiner? I guess I could try to Mock out the code which creates and configures the
replication table and just expect that the already-public methods on Combiner get invoked.
> 
> Christopher Tubbs wrote:
>     Well, you could use the clunky way I described to get the value of the constant from
an anonymous subclass, in your test. But... it sounds like it's not really worth it.

Haha, I'll see if I can mock this to get around opening up these variables, otherwise, it
seems like this isn't a big concern.


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


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