accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "John Vines" <vi...@apache.org>
Subject Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Date Thu, 31 Oct 2013 18:23:01 GMT


> 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.
> 
> John Vines wrote:
>     I was under the impression that this fell under ACCUMULO-1726
> 
> kturner wrote:
>     How is adding a redundant method to the API now related to 1726?
> 
> John Vines wrote:
>     Misunderstood, agree with your assessment.
> 
> Michael Berman wrote:
>     I did remove most of the constructors I added, and I agree it's better without the
bloat, but I am a little concerned about discoverability.  I guess it depends what our long-term
plan is.  If we're expecting that everyone will always use the single-argument ClientConfiguration
constructor, then maybe we should start deprecating the others (and then discoverability is
less of a concern because people will see the options on ClientConfiguration).  But if we
think (String instanceName, String zooKeepers) will continue to be the usual constructor,
then I think it does make sense to have the most common options available in a variant of
that one (although I think it's good not to have every possible permutation of switches cluttering
up the interface).
> 
> Sean Busbey wrote:
>     I think pushing towards the ClientConfiguration constructor is the way to go. I see
an argument for keeping the very simple (String instanceName, String zooKeepers) for examples,
but I'm +0 on it.

Personally, I'm in favor of deprecating ZooKeeperInstance(String instanceName, String zooKeepers)
for the new ZooKeeperInstance(ClientConfiguration)


> 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?
> 
> John Vines wrote:
>     Unless we decide to entirely drop support for the UUID based constructors, then we
should have them mirror any additional functionality.
> 
> kturner wrote:
>     Having ZooKeeperInstance(ClientConfiguration) changes things.  I am assuming this
constructor can replicate the functionality of all the other existing constructors, is this
correct?  I'm thinking we should be moving towards less constructors if ZooKeeperInstance(ClientConfiguration)
exist and not more.
> 
> John Vines wrote:
>     Agreed
> 
> Michael Berman wrote:
>     I think this makes sense, but this relates to my question above about what we think
the usual entrypoint will be and if we should start deprecating all the others.  If we really
think the single-argument ClientConfiguration constructor is the best way to make a ZKInstance,
then we should start encouraging people to migrate to it, right?

Yes


- John


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


On Oct. 31, 2013, 2:35 p.m., John Vines wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14972/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 2:35 p.m.)
> 
> 
> Review request for accumulo and Michael Berman.
> 
> 
> 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 
>   core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java 5b5d041 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ConditionalWriterImpl.java
bb5987d 
>   core/src/main/java/org/apache/accumulo/core/client/impl/MasterClient.java 32c80f9 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ServerClient.java 218bd36 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchReaderIterator.java
0376304 
>   core/src/main/java/org/apache/accumulo/core/client/impl/TabletServerBatchWriter.java
0dd86bf 
>   core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java f07139d

>   core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportPool.java e7dabb5

>   core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloInputFormat.java
bbbd0c3 
>   core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloOutputFormat.java
908b8b3 
>   core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloRowInputFormat.java
fe5003b 
>   core/src/main/java/org/apache/accumulo/core/client/mapred/InputFormatBase.java c796cd2

>   core/src/main/java/org/apache/accumulo/core/client/mapreduce/AccumuloInputFormat.java
1cbb606 
>   core/src/main/java/org/apache/accumulo/core/client/mapreduce/AccumuloOutputFormat.java
727bfec 
>   core/src/main/java/org/apache/accumulo/core/client/mapreduce/AccumuloRowInputFormat.java
992990d 
>   core/src/main/java/org/apache/accumulo/core/client/mapreduce/InputFormatBase.java 13f9708

>   core/src/main/java/org/apache/accumulo/core/client/mapreduce/lib/util/ConfiguratorBase.java
73405c5 
>   core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java 28cb0bd

>   core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/conf/Property.java b6fbdd2 
>   core/src/main/java/org/apache/accumulo/core/security/Credentials.java 0552e7e 
>   core/src/main/java/org/apache/accumulo/core/security/SecurityUtil.java 8add1a7 
>   core/src/main/java/org/apache/accumulo/core/security/ssl/SslConnectionParams.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java e8dd6a2 
>   core/src/main/java/org/apache/accumulo/core/util/shell/Shell.java 4140c8c 
>   core/src/main/java/org/apache/accumulo/core/util/shell/ShellOptionsJC.java cb1f1c8

>   core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java PRE-CREATION

>   core/src/test/java/org/apache/accumulo/core/util/shell/ShellSetInstanceTest.java 23ca13a

>   minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloCluster.java
77776df 
>   minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java 0b6c42c

>   minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloInstance.java
540d7ae 
>   minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterTest.java
3e749ab 
>   server/src/main/java/org/apache/accumulo/server/cli/ClientOnDefaultTable.java 53f5ac2

>   server/src/main/java/org/apache/accumulo/server/cli/ClientOnRequiredTable.java e9e9bf1

>   server/src/main/java/org/apache/accumulo/server/cli/ClientOpts.java 6f3516a 
>   server/src/main/java/org/apache/accumulo/server/client/BulkImporter.java a04765f 
>   server/src/main/java/org/apache/accumulo/server/gc/SimpleGarbageCollector.java 817aa74

>   server/src/main/java/org/apache/accumulo/server/util/TServerUtils.java 1df17fe 
>   server/src/main/resources/web/flot/jquery.flot.js aabc544 
>   test/pom.xml 8343bb2 
>   test/src/test/java/org/apache/accumulo/test/ConditionalWriterTest.java 633ea76 
>   test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 4fbd293 
>   test/src/test/java/org/apache/accumulo/test/functional/AbstractMacIT.java f1a651d 
>   test/src/test/java/org/apache/accumulo/test/functional/BulkIT.java 607f2a5 
>   test/src/test/java/org/apache/accumulo/test/functional/ConcurrencyIT.java c3d3160 
>   test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java c34fff5

>   test/src/test/java/org/apache/accumulo/test/functional/MapReduceIT.java 69825bc 
>   test/src/test/java/org/apache/accumulo/test/functional/ShutdownIT.java 8d58821 
>   test/src/test/java/org/apache/accumulo/test/functional/SimpleMacIT.java 4a37d82 
>   test/src/test/java/org/apache/accumulo/test/functional/SslIT.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/functional/SslWithClientAuthIT.java PRE-CREATION

>   test/src/test/java/org/apache/accumulo/test/functional/SslWithJsseIT.java PRE-CREATION

>   test/src/test/java/org/apache/accumulo/test/util/CertUtils.java PRE-CREATION 
>   test/src/test/java/org/apache/accumulo/test/util/CertUtilsTest.java PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/14972/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> John Vines
> 
>


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