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
Michael Berman 2013-11-08, 14:17


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

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

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.

This is fixed in the followup review.

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: