----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14972/#review28034 -----------------------------------------------------------
Patch should be re-based on master. Some of the diffs against the tests are out of date (ConditionalWriterIT), for instance. Overall, looks okay. Mostly, my outstanding concerns are about easing up on the API a bit with a less restrictive Configuration object in the method signatures, and related improvements. .gitignore <https://reviews.apache.org/r/14972/#comment54515> Drop these .gitignore files. They don't apply to the head of the master branch. core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java <https://reviews.apache.org/r/14972/#comment54516> 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. core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java <https://reviews.apache.org/r/14972/#comment54535> 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. core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java <https://reviews.apache.org/r/14972/#comment54523> This method really needs to be deprecated. It never made sense to return an AccumuloConfiguration here, because AccumuloConfiguration isn't for clients. core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java <https://reviews.apache.org/r/14972/#comment54532> Comment seems to imply that equals will only be called on successful connections. I don't think those semantics should be assumed. I think connection params should be used to compare two of these, because it's more precise and can avoid bugs. Is there a good reason they shouldn't be? core/src/main/java/org/apache/accumulo/core/client/mapreduce/lib/util/ConfiguratorBase.java <https://reviews.apache.org/r/14972/#comment54534> 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. core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java <https://reviews.apache.org/r/14972/#comment54536> 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.) core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java <https://reviews.apache.org/r/14972/#comment54537> Again, not needed if we can load from general Configuration object. If necessary, put a toConfiguration() method on AccumuloConfiguration. core/src/main/java/org/apache/accumulo/core/util/ThriftUtil.java <https://reviews.apache.org/r/14972/#comment54539> 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. core/src/test/java/org/apache/accumulo/core/conf/ClientConfigurationTest.java <https://reviews.apache.org/r/14972/#comment54540> Should test expected config before and after serialization. Otherwise, if it's only correct after deserialization, we won't catch it. - Christopher Tubbs 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 > >
