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 04:56:51 GMT


> On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote:
> > This is a partial review. I didn't examine all the implementation stuff. One question
I had was whether or not this adds a new dependency on protocol buffers? If so, where is the
pom.xml change for that? Another patch?

Yes, I use protocol buffers. No, it does not require any dependencies. Like thrift, it just
generates new Java files which are included, but there is no runtime library required to use
them. They are self-contained. Hadoop has a maven plugin that could automate the re-generation
of them when the definition needs to be updated, but I didn't bother trying to wire that up
since, last I checked, it wasn't in central (and would be a bigger pain to force devs to install
over just installing protoc)


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

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.


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

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.


> On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/client/admin/ReplicationOperations.java,
line 77
> > <https://reviews.apache.org/r/22003/diff/1/?file=598217#file598217line77>
> >
> >     What does "referenced files" mean? Referenced how? This needs javadoc update
to clarify, and the answer may yield a better method name. At the very least, a getReferencedFile
would be helpful to identify it clearly as a getter without side-effects.

Noted -- same back-story about drain applies here. I can make better docs, maybe a better
name too.


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

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


> On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/data/Mutation.java, line 919
> > <https://reviews.apache.org/r/22003/diff/1/?file=598233#file598233line919>
> >
> >     Please apply project formatter to strip trailing whitespace on blank lines.
(commenting here, but applies to entire patch set)

Yes, I intended to reformat everything before I commit. It's a waste of time for me to incrementally
re-apply the formatter after every commit I make (assuming I goof somewhere every time).


> On June 4, 2014, 8:59 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/data/thrift/TConditionalMutation.java,
lines 617-646
> > <https://reviews.apache.org/r/22003/diff/1/?file=598236#file598236line617>
> >
> >     Next time, please exclude generated thrift from review. :)

Hahaha, psh. Sorry, I didn't think about excluding them when I was copy/pasting these diffs.


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

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?


- 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