> 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.
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,
and if I
want I can just drop that same file into ~/.accumulo and have everyone pick
it up automatically.
> On Nov. 1, 2013, 9: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.
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?
- 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
>
>
