accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubbsii...@apache.org>
Subject Re: Review Request 22003: Replication: Client-facing changes
Date Thu, 05 Jun 2014 17:17:05 GMT


> On June 4, 2014, 4: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.

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)


> On June 4, 2014, 4: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.

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


> On June 4, 2014, 4: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?

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.


> On June 4, 2014, 4: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 :)

A ticket would be nice, for future work.


> On June 4, 2014, 4: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?

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.


- Christopher


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


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