Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Accumulo >> mail # dev >> Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms


Copy link to this message
-
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms


> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 102
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line102>
> >
> >     I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor.  The constructor that takes the config object nicely covers this use case.

I was under the impression that this fell under ACCUMULO-1726
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 164
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line164>
> >
> >     Probably do not need to add this method.  I think most users connect using the instance name.  If they really want to connect using the instance id and pass config, can they use the constructor that only takes config?

Unless we decide to entirely drop support for the UUID based constructors, then we should have them mirror any additional functionality.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ServerClient.java, line 112
> > <https://reviews.apache.org/r/14972/diff/1/?file=371856#file371856line112>
> >
> >     Communication failures are the norm, this should probably stay as debug?

I could see it staying as debug
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java, line 385
> > <https://reviews.apache.org/r/14972/diff/1/?file=371882#file371882line385>
> >
> >     Mini is intended to be user facing and allow user to test their code against Accumulo.  We keep exposing methods in the minicluster public API inorder for Accumulo to test Accumulo.  I worry that the more we expose in its API the more we will box ourselves in for future changes to mini (like speeding it up some way).  
> >    
> >     This is a general concern I have, not specific to this change.  This change is following the general trend for mini.  I have not had time to pursue, but I have wondered if it would be worthwhile to create a mini for internal use and one for external use (with a more minimal API).  This may not be worthwhile.  
> >

This sounds like something that should be written up in it's own ticket
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > test/pom.xml, line 27
> > <https://reviews.apache.org/r/14972/diff/1/?file=371892#file371892line27>
> >
> >     All export control issues w/ this are resolved?

Yes, we are all set with crypto based export for both including bouncycastle and RFile encryption
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > test/src/test/java/org/apache/accumulo/test/ConditionalWriterTest.java, line 106
> > <https://reviews.apache.org/r/14972/diff/1/?file=371893#file371893line106>
> >
> >     Whats going on w/ the white space changes in this file?  Are you using an incorrect config or was this file checked in w/ incorrect formatting?

Probably missed the config update with the git switch like I did
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
On Oct. 26, 2013, 2:36 a.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14972/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2013, 2:36 a.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-1009
>     https://issues.apache.org/jira/browse/ACCUMULO-1009
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Michael Berman's October 13 patch for ACCUMULO-1009
>
>
> Diffs
> -----
>
>   .gitignore 1ffa452
>   core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java 9247d56