> 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. > > 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) > } > 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. > On Nov. 1, 2013, 5: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 I'll retract this comment. I was thinking this was public API, but it's not. It's in the utility class. > On Nov. 1, 2013, 5:37 p.m., Christopher Tubbs wrote: > > core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java, lines > > 99-127 > > <https://reviews.apache.org/r/14972/diff/1/?file=371876#file371876line99> > > > > 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. > > Michael Berman wrote: > I agree it would be good to push ClientConfiguration deeper into the > stack, but at the moment it's not a trivial change, since some of the paths > that are making connections never had a ClientConfiguration in the first > place. Since it doesn't feel critical to this changeset, in the interest in > reducing the number of merges I need to do between now and feature freeze, > I'd like to address that as a separate issue. > > Right now it looks to me like all of the entrypoints are referenced. Was > there a particular one you saw that you thought could be dropped? You're right. Nothing here is urgent. So long as a ticket is opened to clean it up at some point. > On Nov. 1, 2013, 5: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. 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: > > ----------------------------------------------------------- > 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 > >
