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
Christopher Tubbs 2013-11-07, 23:48


> On Nov. 1, 2013, 5: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.

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.
> On Nov. 1, 2013, 5: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.

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.

I'll retract this comment. I was thinking this was public API, but it's not. It's in the utility class.

You're right. Nothing here is urgent. So long as a ticket is opened to clean it up at some point.

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