accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Berman" <mber...@sqrrl.com>
Subject Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Date Fri, 08 Nov 2013 14:17:50 GMT


> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java, lines 170-171
> > <https://reviews.apache.org/r/14972/diff/1/?file=371852#file371852line170>
> >
> >     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.
> 
> Michael Berman wrote:
>     The search path isn't really a list of directory paths to search, it's a list of
full config file paths to look for, and the first one we find wins.  So are you saying you'd
prefer ~/.accumulo/accumulo-client.conf; /etc/accumulo/accumulo-client.conf; etc?  I don't
think I have a strong preference, but it does seem like a lot of "accumulo" in the path. 
I went with ~/.accumulo/config because that's what was suggested in some jira issue, but I
could definitely see making that one ~/.accumulo/client.conf so it has the same default filename
as the other two locations.
>     
>     The thing that's nice about the --config-file CLI option is that it's in the client.conf
format (key=value pairs with the full property key), whereas the jcommander option would need
to be in terms of command line switches ("-i instanceName -z zooHost:2181" or whatever). 
The client.conf format is supported universally across all ZooKeeperInstances, even for external
client apps.  The jcommander solution is fine, but would be specific to each particular client
app.  This means that if want common client config for the accumulo shell, the accumulo admin
command, and some arbitrary third party console client, if we were using the CLI options-based
file, I would need to hope that the third party tool happens to use the same switches as accumulo's
tools, or I need to have separate versions of the file for each.  If we use the ClientConfiguration
properties, then I can use the same file for everyone, the third party tool's implementation
to take advantage of that file is trivial, an
 d if I want I can just drop that same file into ~/.accumulo and have everyone pick it up
automatically.
> 
> Christopher Tubbs wrote:
>     No, would not prefer that it look for "accumulo-client.conf" in the search path.
I guess I just misunderstood the description. I think what you have is fine, now that I understand
it.
>     
>     I prefer strictly Java property files... I'm not sure what a "client.conf format"
is, but hopefully it's a Java property file. I think I understand and agree with what you're
saying about the limitations of using JCommander.

Ok cool.  Yeah, client.conf format is just Java property file (actually, commons PropertiesConfiguration's
extension thereof, which supports ${variable} replacement, includes, etc.), and I meant specifically
one with the keys we recognize.


> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line
194
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line194>
> >
> >     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.
> 
> John Vines wrote:
>     That sounds fairly utilitarian, but having the dedicated builder methods we get from
having a dedicated Configuration class will make it substantially easier for users to use
the configuration and explore the options we provide to them. They could theoretically use
the ClientConfiguration with a Configuration API, but then it makes it harder for the user
to find it. It may be a little bit redundant, but I think having 2 signatures, one for Configuration
and one for ClientConfiguration, to make it easier for the user to find things.
> 
> kturner wrote:
>     I think we can have a constructor that take a a generic config object and still make
it easy for users to find something w/ helper methods via java doc.  Something like the following.
>     
>     class ClientConfiguration extends Configuration {
>       //lots of convient methods specific to accumulo config
>     }
>     
>     class ZookeeperInstance {
>     /**
>      * @param config see {@link ClientConfiguration} it extends Configuration and offer
convenience methods specific to Accumulo
>      */
>      ZookeeperInstance(Configuration config)
>     }
>
> 
> Christopher Tubbs wrote:
>     I'm not strongly opposed to an overloaded method option, but I prefer the configuration
object (the thing that carries the configuration) be independent of the discovery of the configuration
keys or the utility methods. I know that's not the precedent I set with AccumuloConfiguration...
but I've learned a lot since I wrote that, and I really dislike how they are so tightly coupled
now. I think JavaDoc is fine for now; I have some partially complete ideas for improving configuration
in the next dev cycle.

Updated according to Keith's suggestion in the followup review.


> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/client/mapreduce/lib/util/ConfiguratorBase.java,
line 302
> > <https://reviews.apache.org/r/14972/diff/1/?file=371869#file371869line302>
> >
> >     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.
> 
> John Vines wrote:
>     Please see discussion https://reviews.apache.org/r/14972/#comment53742
> 
> Christopher Tubbs wrote:
>     I'll retract this comment. I was thinking this was public API, but it's not. It's
in the utility class.

I did end up cleaning it up following that discussion upthread, since it was bugging me anyway.

Are you sure this isn't in the public API, though?  I figured this is the method people would
use in their own MR jobs, for passing configuration to accumulo's In/OutputFormats.  If I've
misunderstood how this code gets consumed, and it's not really public, I'll skip the deprecation
cycle and just get rid of the old ones now.


> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java, lines
117-122
> > <https://reviews.apache.org/r/14972/diff/1/?file=371871#file371871line117>
> >
> >     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.)

This is fixed in the followup review.


> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java, line
38
> > <https://reviews.apache.org/r/14972/diff/1/?file=371879#file371879line38>
> >
> >     Should test expected config before and after serialization. Otherwise, if it's
only correct after deserialization, we won't catch it.
> 
> Michael Berman wrote:
>     Well, if it's not correct before serialization, the previous unit test will fail.
 I don't have a problem sticking in a sanity check test of the pre-serialization config, but
I guess it depends how unitary we want our unit tests to be.
> 
> Christopher Tubbs wrote:
>     Unit test correctness shouldn't depend on the correctness of other unit tests. What
if that other test was deleted or modified? Plus, it's a bit more expressive and clear what
one expects.

Fair enough, I'll update it on the followup review.


- Michael


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


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