accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Christopher Tubbs" <ctubb...@apache.org>
Subject Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Date Fri, 01 Nov 2013 21:37:20 GMT

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


Patch should be re-based on master. Some of the diffs against the tests are out of date (ConditionalWriterIT),
for instance. Overall, looks okay. Mostly, my outstanding concerns are about easing up on
the API a bit with a less restrictive Configuration object in the method signatures, and related
improvements.


.gitignore
<https://reviews.apache.org/r/14972/#comment54515>

    Drop these .gitignore files. They don't apply to the head of the master branch.



core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java
<https://reviews.apache.org/r/14972/#comment54516>

    I believe that jcommander has a built-in way of getting properties from a configuration
file. We don't need to bake it in as an additional parameter and interpret it ourselves.
    
    I do like the defaulting to ~/.accumulo/config, but I'm not sure how ACCUMULO_CLIENT_CONF_PATH
is expected to be searched. Is it looking for a specific filename? If so, and you can specify
an arbitrary path, it should probably look for one with an accumulo-specific name, like accumulo-client.conf;
However, I think it would be easier to omit this entirely, and rely on the jcommander feature
to specify a config file.



core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
<https://reviews.apache.org/r/14972/#comment54535>

    Can we make this method signature, and others like it, take just a Configuration object
(commons Configuration) rather than an Accumulo-specific one? I'd rather not be tied to our
interface when we could be more general and allow users to construct configuration by all
the options available to them using that standard library.



core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
<https://reviews.apache.org/r/14972/#comment54523>

    This method really needs to be deprecated. It never made sense to return an AccumuloConfiguration
here, because AccumuloConfiguration isn't for clients.



core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java
<https://reviews.apache.org/r/14972/#comment54532>

    Comment seems to imply that equals will only be called on successful connections. I don't
think those semantics should be assumed. I think connection params should be used to compare
two of these, because it's more precise and can avoid bugs.
    
    Is there a good reason they shouldn't be?



core/src/main/java/org/apache/accumulo/core/client/mapreduce/lib/util/ConfiguratorBase.java
<https://reviews.apache.org/r/14972/#comment54534>

    There's really too many methods here. If we're going to provide an alternate way to set
configuration on a M/R job, we shouldn't take some properties, plus a configuration object
with other properties... we should just take the configuration object and expect it to include
all the necessary options.



core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java
<https://reviews.apache.org/r/14972/#comment54536>

    org.apache.commons.configuration.PropertiesConfiguration does this for you.
    
    If we need this behavior, we can/should have a method to wrap a general Configuration
object with the ClientConfiguration interface. (Alternatively, put all the interesting interface
bits in the Properties rather than on the config object.)



core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java
<https://reviews.apache.org/r/14972/#comment54537>

    Again, not needed if we can load from general Configuration object. If necessary, put
a toConfiguration() method on AccumuloConfiguration.



core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java
<https://reviews.apache.org/r/14972/#comment54539>

    It'd be better if these method signatures were simplified a bit. Perhaps it can just take
a client configuration?
    
    Also, we can remove any of these that we aren't using. This isn't public API. So, if we
don't use the ones with longs, and use the timeoutProperty instead, then we should drop those
using longs.



core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java
<https://reviews.apache.org/r/14972/#comment54540>

    Should test expected config before and after serialization. Otherwise, if it's only correct
after deserialization, we won't catch it.


- Christopher Tubbs


On Oct. 31, 2013, 10:35 a.m., John Vines wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14972/
> -----------------------------------------------------------
> 
> (Updated Oct. 31, 2013, 10:35 a.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