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