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 17:44:58 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)

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.


> 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.
> 
> Josh Elser wrote:
>     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.
> 
> Christopher Tubbs wrote:
>     I like the drain analogy, but from an API perspective, the user doesn't really need
to know here whether the implementation flushes queued replication tasks or just waits for
existing tasks to complete, or some other implementation variant (and that could actually
change over time), and the semantics appear to be strikingly similar to coreutils' /bin/sync
>     
>     If all you want to provide is a wait loop, perhaps another option is to just provide
an "boolean isFullyReplicated();" method or similar so users can write their own wait code.
Alternatively, something very explicit might work rather than an analogy, like "waitForFullReplication()"
or similar.
>     
>     With good javadocs, drain works, but I think it could be more clear.
>     
>     (Also, the overloaded method with the additional files parameter needs to be explained
a bit more.)

I improved the documentation on these methods already. I'm apt to let this ride for now and
see if it's still misleading or confusing. I can add the isFullyReplicated method as I can
see the merits of having that method as well and it's practically written for me already with
the implementation of drain.


> On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/conf/Property.java, line 433
> > <https://reviews.apache.org/r/22003/diff/1/?file=598231#file598231line433>
> >
> >     Target or targets? enum and key should match in number.
> 
> Josh Elser wrote:
>     I did it this way because it is a prefix. and thus there would be many "table.replication.target."
prefixes, but each key-value is singular and represents a single target. Do you still think
this is confusing?
> 
> Christopher Tubbs wrote:
>     By convention, all our other property prefixes look like: <SINGULAR_THING_THAT_IS_PREFIXED>_PREFIX
>     
>     The reason for this is because it doesn't prefix the set. Rather, it prefixes each
singular element of the set. Further, the enum and the key for the enum are really identifiers
for the same thing (in a slightly different form), so it's very confusing for them to have
different number. Grammatically speaking, I think the convention is better.

Ok, I'll remove the plural from the property key.


> 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.
> 
> Josh Elser wrote:
>     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 :)
> 
> Christopher Tubbs wrote:
>     A ticket would be nice, for future work.

ACCUMULO-2860


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

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.


- 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