You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by John Vines <vi...@apache.org> on 2013/10/26 04:36:42 UTC
Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/
-----------------------------------------------------------
Review request for accumulo.
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
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
> 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.
>
> Michael Berman wrote:
> 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.
>
> Christopher Tubbs wrote:
> ConfiguratorBase, along with all the classes in this util package, is an internal util class shared between mapred and mapreduce implementations for the implementation of the static configurator methods on the specific {Input,Output}Format. The public API for this is the static methods on the actual InputFormat and OutputFormat classes.
>
> The classes could be consumed by alternate implementations of jobs, like those in Hama BSP, but it would be risky to rely on them, as I never intended them to be part of the public API. That's why I put them in a util sub-package and called them inside the static configurator methods.
>
> The fact that they take the "implementingClass" parameter should also be an indicator they are internal helper code, and not public API.
Oh yeah, of course...I was confusing the methods on ConfiguratorBase with the similar ones that call them from the {Input,Output}Formats.
Do you think it's worth deprecating those methods on ConfiguratorBase anyway, or should I just drop the ones that are no longer used?
- 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
> 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Christopher Tubbs <ct...@apache.org>.
> 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by ke...@deenlo.com.
> 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.
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)
}
> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, lines 313-319
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line313>
> >
> > This method really needs to be deprecated. It never made sense to return an AccumuloConfiguration here, because AccumuloConfiguration isn't for clients.
Agreed, and thats probably a separate ticket?
- kturner
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
> 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.
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.
- 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
> 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Christopher Tubbs <ct...@apache.org>.
> 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
>
> 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.
>
> Michael Berman wrote:
> 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.
ConfiguratorBase, along with all the classes in this util package, is an internal util class shared between mapred and mapreduce implementations for the implementation of the static configurator methods on the specific {Input,Output}Format. The public API for this is the static methods on the actual InputFormat and OutputFormat classes.
The classes could be consumed by alternate implementations of jobs, like those in Hama BSP, but it would be risky to rely on them, as I never intended them to be part of the public API. That's why I put them in a util sub-package and called them inside the static configurator methods.
The fact that they take the "implementingClass" parameter should also be an indicator they are internal helper code, and not public API.
- 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by John Vines <vi...@apache.org>.
> 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.
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.
- John
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by John Vines <vi...@apache.org>.
> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java, lines 62-64
> > <https://reviews.apache.org/r/14972/diff/1/?file=371859#file371859line62>
> >
> > 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?
This issue was already opened by Keith
> 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.
Please see discussion https://reviews.apache.org/r/14972/#comment53742
> On Nov. 1, 2013, 9:37 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java, lines 299-307
> > <https://reviews.apache.org/r/14972/diff/1/?file=371871#file371871line299>
> >
> > Again, not needed if we can load from general Configuration object. If necessary, put a toConfiguration() method on AccumuloConfiguration.
This seems to hedge on the decision of https://reviews.apache.org/r/14972/#comment54535
Additionally, I agreed with your point back in September to keep the AccumuloConfiguration and ClientConfigurations from bleeding together. I think adding a toConfiguration() method would have unintended side effects of encouraging it, though.
- John
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by John Vines <vi...@apache.org>.
-----------------------------------------------------------
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
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Sean Busbey <se...@manvsbeard.com>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 102
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line102>
> >
> > I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor. The constructor that takes the config object nicely covers this use case.
>
> John Vines wrote:
> I was under the impression that this fell under ACCUMULO-1726
>
> kturner wrote:
> How is adding a redundant method to the API now related to 1726?
>
> John Vines wrote:
> Misunderstood, agree with your assessment.
>
> Michael Berman wrote:
> I did remove most of the constructors I added, and I agree it's better without the bloat, but I am a little concerned about discoverability. I guess it depends what our long-term plan is. If we're expecting that everyone will always use the single-argument ClientConfiguration constructor, then maybe we should start deprecating the others (and then discoverability is less of a concern because people will see the options on ClientConfiguration). But if we think (String instanceName, String zooKeepers) will continue to be the usual constructor, then I think it does make sense to have the most common options available in a variant of that one (although I think it's good not to have every possible permutation of switches cluttering up the interface).
I think pushing towards the ClientConfiguration constructor is the way to go. I see an argument for keeping the very simple (String instanceName, String zooKeepers) for examples, but I'm +0 on it.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java, line 161
> > <https://reviews.apache.org/r/14972/diff/1/?file=371870#file371870line161>
> >
> > there is also $ACCUMULO_CONF_DIR (not sure if I have the var name exactly correct)
>
> Michael Berman wrote:
> I was wondering about this... The code that I generalized that from only paid attention to $ACCUMULO_HOME and not the other one, and as I was looking through the codebase it seemed about 50/50 whether a given code path respected $ACCUMULO_CONF_DIR. What is the intention there? Is the plan for both to co-exist (and $ACCUMULO_CONF_DIR overrides $ACCUMULO_HOME/conf)? Should we go through and make sure it's universally supported?
everything should be using $ACCUMULO_CONF_DIR. if there are places that don't contain it, please file a bug that points to as many of them as possible. If you are adding in new code, please use it.
There are some existing bugs for it not being set correctly in tests.
I believe one of config.sh or bin/accumulo is responsible for handling the fall back to $ACCUMULO_HOME/conf when $ACCUMULO_CONF_DIR doesn't exist.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java, line 385
> > <https://reviews.apache.org/r/14972/diff/1/?file=371882#file371882line385>
> >
> > Mini is intended to be user facing and allow user to test their code against Accumulo. We keep exposing methods in the minicluster public API inorder for Accumulo to test Accumulo. I worry that the more we expose in its API the more we will box ourselves in for future changes to mini (like speeding it up some way).
> >
> > This is a general concern I have, not specific to this change. This change is following the general trend for mini. I have not had time to pursue, but I have wondered if it would be worthwhile to create a mini for internal use and one for external use (with a more minimal API). This may not be worthwhile.
> >
>
> John Vines wrote:
> This sounds like something that should be written up in it's own ticket
>
> Michael Berman wrote:
> I'm not sure I agree that this is exclusive to us testing accumulo... On a big cluster, system props can be set in accumulo-env.sh, so it makes sense to me that there would also be a way to set them in a minicluster.
>
> That said, I don't think we actually need it for the SSL changeset quite yet. I added it so that I could test JSSE, but it turns out that doesn't work at all with the ZK/JSSE incompatibility, so if it's controversial we could take it out. But I'll want it back in if ZOOKEEPER-1554 gets addressed and we want to flesh out JSSE support.
+1 for removal.
I'd rather see discussion on the dev@ list about Mini internal-vs-external than a ticket.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > server/src/main/resources/web/flot/jquery.flot.js, line 1560
> > <https://reviews.apache.org/r/14972/diff/1/?file=371891#file371891line1560>
> >
> > why was this change made? What does it have to do w/ ssl?
>
> Michael Berman wrote:
> It has nothing to do with this particular changeset except that I hate seeing errors in my IDE while I'm working on things. I have no problem taking it out of this commit, but I would love if a committer could fix it one way or another.
Please put formatting / unrelated errors into an different JIRA/commit.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by John Vines <vi...@apache.org>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 102
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line102>
> >
> > I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor. The constructor that takes the config object nicely covers this use case.
I was under the impression that this fell under ACCUMULO-1726
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 164
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line164>
> >
> > Probably do not need to add this method. I think most users connect using the instance name. If they really want to connect using the instance id and pass config, can they use the constructor that only takes config?
Unless we decide to entirely drop support for the UUID based constructors, then we should have them mirror any additional functionality.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ServerClient.java, line 112
> > <https://reviews.apache.org/r/14972/diff/1/?file=371856#file371856line112>
> >
> > Communication failures are the norm, this should probably stay as debug?
I could see it staying as debug
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java, line 385
> > <https://reviews.apache.org/r/14972/diff/1/?file=371882#file371882line385>
> >
> > Mini is intended to be user facing and allow user to test their code against Accumulo. We keep exposing methods in the minicluster public API inorder for Accumulo to test Accumulo. I worry that the more we expose in its API the more we will box ourselves in for future changes to mini (like speeding it up some way).
> >
> > This is a general concern I have, not specific to this change. This change is following the general trend for mini. I have not had time to pursue, but I have wondered if it would be worthwhile to create a mini for internal use and one for external use (with a more minimal API). This may not be worthwhile.
> >
This sounds like something that should be written up in it's own ticket
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > test/pom.xml, line 27
> > <https://reviews.apache.org/r/14972/diff/1/?file=371892#file371892line27>
> >
> > All export control issues w/ this are resolved?
Yes, we are all set with crypto based export for both including bouncycastle and RFile encryption
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > test/src/test/java/org/apache/accumulo/test/ConditionalWriterTest.java, line 106
> > <https://reviews.apache.org/r/14972/diff/1/?file=371893#file371893line106>
> >
> > Whats going on w/ the white space changes in this file? Are you using an incorrect config or was this file checked in w/ incorrect formatting?
Probably missed the config update with the git switch like I did
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
On Oct. 26, 2013, 2:36 a.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14972/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2013, 2:36 a.m.)
>
>
> Review request for accumulo.
>
>
> 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by ke...@deenlo.com.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 102
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line102>
> >
> > I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor. The constructor that takes the config object nicely covers this use case.
>
> John Vines wrote:
> I was under the impression that this fell under ACCUMULO-1726
How is adding a redundant method to the API now related to 1726?
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 164
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line164>
> >
> > Probably do not need to add this method. I think most users connect using the instance name. If they really want to connect using the instance id and pass config, can they use the constructor that only takes config?
>
> John Vines wrote:
> Unless we decide to entirely drop support for the UUID based constructors, then we should have them mirror any additional functionality.
Having ZooKeeperInstance(ClientConfiguration) changes things. I am assuming this constructor can replicate the functionality of all the other existing constructors, is this correct? I'm thinking we should be moving towards less constructors if ZooKeeperInstance(ClientConfiguration) exist and not more.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
On Oct. 26, 2013, 2:36 a.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14972/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2013, 2:36 a.m.)
>
>
> Review request for accumulo.
>
>
> 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java, line 385
> > <https://reviews.apache.org/r/14972/diff/1/?file=371882#file371882line385>
> >
> > Mini is intended to be user facing and allow user to test their code against Accumulo. We keep exposing methods in the minicluster public API inorder for Accumulo to test Accumulo. I worry that the more we expose in its API the more we will box ourselves in for future changes to mini (like speeding it up some way).
> >
> > This is a general concern I have, not specific to this change. This change is following the general trend for mini. I have not had time to pursue, but I have wondered if it would be worthwhile to create a mini for internal use and one for external use (with a more minimal API). This may not be worthwhile.
> >
>
> John Vines wrote:
> This sounds like something that should be written up in it's own ticket
>
> Michael Berman wrote:
> I'm not sure I agree that this is exclusive to us testing accumulo... On a big cluster, system props can be set in accumulo-env.sh, so it makes sense to me that there would also be a way to set them in a minicluster.
>
> That said, I don't think we actually need it for the SSL changeset quite yet. I added it so that I could test JSSE, but it turns out that doesn't work at all with the ZK/JSSE incompatibility, so if it's controversial we could take it out. But I'll want it back in if ZOOKEEPER-1554 gets addressed and we want to flesh out JSSE support.
>
> Sean Busbey wrote:
> +1 for removal.
>
> I'd rather see discussion on the dev@ list about Mini internal-vs-external than a ticket.
>
> John Vines wrote:
> Sure, either way it's not in the scope of this ticket.
ACCUMULO-1838
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > server/src/main/resources/web/flot/jquery.flot.js, line 1560
> > <https://reviews.apache.org/r/14972/diff/1/?file=371891#file371891line1560>
> >
> > why was this change made? What does it have to do w/ ssl?
>
> Michael Berman wrote:
> It has nothing to do with this particular changeset except that I hate seeing errors in my IDE while I'm working on things. I have no problem taking it out of this commit, but I would love if a committer could fix it one way or another.
>
> Sean Busbey wrote:
> Please put formatting / unrelated errors into an different JIRA/commit.
ACCUMULO-1837
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by John Vines <vi...@apache.org>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 102
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line102>
> >
> > I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor. The constructor that takes the config object nicely covers this use case.
>
> John Vines wrote:
> I was under the impression that this fell under ACCUMULO-1726
>
> kturner wrote:
> How is adding a redundant method to the API now related to 1726?
>
> John Vines wrote:
> Misunderstood, agree with your assessment.
>
> Michael Berman wrote:
> I did remove most of the constructors I added, and I agree it's better without the bloat, but I am a little concerned about discoverability. I guess it depends what our long-term plan is. If we're expecting that everyone will always use the single-argument ClientConfiguration constructor, then maybe we should start deprecating the others (and then discoverability is less of a concern because people will see the options on ClientConfiguration). But if we think (String instanceName, String zooKeepers) will continue to be the usual constructor, then I think it does make sense to have the most common options available in a variant of that one (although I think it's good not to have every possible permutation of switches cluttering up the interface).
>
> Sean Busbey wrote:
> I think pushing towards the ClientConfiguration constructor is the way to go. I see an argument for keeping the very simple (String instanceName, String zooKeepers) for examples, but I'm +0 on it.
Personally, I'm in favor of deprecating ZooKeeperInstance(String instanceName, String zooKeepers) for the new ZooKeeperInstance(ClientConfiguration)
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 164
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line164>
> >
> > Probably do not need to add this method. I think most users connect using the instance name. If they really want to connect using the instance id and pass config, can they use the constructor that only takes config?
>
> John Vines wrote:
> Unless we decide to entirely drop support for the UUID based constructors, then we should have them mirror any additional functionality.
>
> kturner wrote:
> Having ZooKeeperInstance(ClientConfiguration) changes things. I am assuming this constructor can replicate the functionality of all the other existing constructors, is this correct? I'm thinking we should be moving towards less constructors if ZooKeeperInstance(ClientConfiguration) exist and not more.
>
> John Vines wrote:
> Agreed
>
> Michael Berman wrote:
> I think this makes sense, but this relates to my question above about what we think the usual entrypoint will be and if we should start deprecating all the others. If we really think the single-argument ClientConfiguration constructor is the best way to make a ZKInstance, then we should start encouraging people to migrate to it, right?
Yes
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by John Vines <vi...@apache.org>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java, line 385
> > <https://reviews.apache.org/r/14972/diff/1/?file=371882#file371882line385>
> >
> > Mini is intended to be user facing and allow user to test their code against Accumulo. We keep exposing methods in the minicluster public API inorder for Accumulo to test Accumulo. I worry that the more we expose in its API the more we will box ourselves in for future changes to mini (like speeding it up some way).
> >
> > This is a general concern I have, not specific to this change. This change is following the general trend for mini. I have not had time to pursue, but I have wondered if it would be worthwhile to create a mini for internal use and one for external use (with a more minimal API). This may not be worthwhile.
> >
>
> John Vines wrote:
> This sounds like something that should be written up in it's own ticket
>
> Michael Berman wrote:
> I'm not sure I agree that this is exclusive to us testing accumulo... On a big cluster, system props can be set in accumulo-env.sh, so it makes sense to me that there would also be a way to set them in a minicluster.
>
> That said, I don't think we actually need it for the SSL changeset quite yet. I added it so that I could test JSSE, but it turns out that doesn't work at all with the ZK/JSSE incompatibility, so if it's controversial we could take it out. But I'll want it back in if ZOOKEEPER-1554 gets addressed and we want to flesh out JSSE support.
>
> Sean Busbey wrote:
> +1 for removal.
>
> I'd rather see discussion on the dev@ list about Mini internal-vs-external than a ticket.
Sure, either way it's not in the scope of this ticket.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
On Oct. 29, 2013, 3:25 p.m., John Vines wrote:
> > I suggested some test in this comment https://issues.apache.org/jira/browse/ACCUMULO-1009?focusedCommentId=13771990&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13771990
> >
> > Do you know if listscans still show the client IP properly with this change?
>
> Michael Berman wrote:
> Oh yeah, thanks for the reminder. I will add an appropriate test.
>
> I'll also add all the @sinces and other updates discussed above. I'll file a fresh review with the next version, now that I have an account here, so Vines stops getting all the notifications.
So, it definitely still works, but I'm not sure how best to write an automated test for it. The existing test of listscans made no attempt to verify that the client address was reasonable...I've added a test to make sure it's of the form host:port, but I don't want to check that it's my local IP or hostname because there's so much potential for variation in how a loopback adapter reports its address. I have manually verified that it works right, though. Do you think that's enough?
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by John Vines <vi...@apache.org>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 102
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line102>
> >
> > I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor. The constructor that takes the config object nicely covers this use case.
>
> John Vines wrote:
> I was under the impression that this fell under ACCUMULO-1726
>
> kturner wrote:
> How is adding a redundant method to the API now related to 1726?
Misunderstood, agree with your assessment.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 164
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line164>
> >
> > Probably do not need to add this method. I think most users connect using the instance name. If they really want to connect using the instance id and pass config, can they use the constructor that only takes config?
>
> John Vines wrote:
> Unless we decide to entirely drop support for the UUID based constructors, then we should have them mirror any additional functionality.
>
> kturner wrote:
> Having ZooKeeperInstance(ClientConfiguration) changes things. I am assuming this constructor can replicate the functionality of all the other existing constructors, is this correct? I'm thinking we should be moving towards less constructors if ZooKeeperInstance(ClientConfiguration) exist and not more.
Agreed
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
On Oct. 26, 2013, 2:36 a.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14972/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2013, 2:36 a.m.)
>
>
> Review request for accumulo.
>
>
> 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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 164
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line164>
> >
> > Probably do not need to add this method. I think most users connect using the instance name. If they really want to connect using the instance id and pass config, can they use the constructor that only takes config?
>
> John Vines wrote:
> Unless we decide to entirely drop support for the UUID based constructors, then we should have them mirror any additional functionality.
>
> kturner wrote:
> Having ZooKeeperInstance(ClientConfiguration) changes things. I am assuming this constructor can replicate the functionality of all the other existing constructors, is this correct? I'm thinking we should be moving towards less constructors if ZooKeeperInstance(ClientConfiguration) exist and not more.
>
> John Vines wrote:
> Agreed
I think this makes sense, but this relates to my question above about what we think the usual entrypoint will be and if we should start deprecating all the others. If we really think the single-argument ClientConfiguration constructor is the best way to make a ZKInstance, then we should start encouraging people to migrate to it, right?
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 102
> > <https://reviews.apache.org/r/14972/diff/1/?file=371853#file371853line102>
> >
> > I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor. The constructor that takes the config object nicely covers this use case.
>
> John Vines wrote:
> I was under the impression that this fell under ACCUMULO-1726
>
> kturner wrote:
> How is adding a redundant method to the API now related to 1726?
>
> John Vines wrote:
> Misunderstood, agree with your assessment.
I did remove most of the constructors I added, and I agree it's better without the bloat, but I am a little concerned about discoverability. I guess it depends what our long-term plan is. If we're expecting that everyone will always use the single-argument ClientConfiguration constructor, then maybe we should start deprecating the others (and then discoverability is less of a concern because people will see the options on ClientConfiguration). But if we think (String instanceName, String zooKeepers) will continue to be the usual constructor, then I think it does make sense to have the most common options available in a variant of that one (although I think it's good not to have every possible permutation of switches cluttering up the interface).
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ServerClient.java, line 112
> > <https://reviews.apache.org/r/14972/diff/1/?file=371856#file371856line112>
> >
> > Communication failures are the norm, this should probably stay as debug?
>
> John Vines wrote:
> I could see it staying as debug
Sold.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java, line 161
> > <https://reviews.apache.org/r/14972/diff/1/?file=371870#file371870line161>
> >
> > there is also $ACCUMULO_CONF_DIR (not sure if I have the var name exactly correct)
I was wondering about this... The code that I generalized that from only paid attention to $ACCUMULO_HOME and not the other one, and as I was looking through the codebase it seemed about 50/50 whether a given code path respected $ACCUMULO_CONF_DIR. What is the intention there? Is the plan for both to co-exist (and $ACCUMULO_CONF_DIR overrides $ACCUMULO_HOME/conf)? Should we go through and make sure it's universally supported?
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java, line 385
> > <https://reviews.apache.org/r/14972/diff/1/?file=371882#file371882line385>
> >
> > Mini is intended to be user facing and allow user to test their code against Accumulo. We keep exposing methods in the minicluster public API inorder for Accumulo to test Accumulo. I worry that the more we expose in its API the more we will box ourselves in for future changes to mini (like speeding it up some way).
> >
> > This is a general concern I have, not specific to this change. This change is following the general trend for mini. I have not had time to pursue, but I have wondered if it would be worthwhile to create a mini for internal use and one for external use (with a more minimal API). This may not be worthwhile.
> >
>
> John Vines wrote:
> This sounds like something that should be written up in it's own ticket
I'm not sure I agree that this is exclusive to us testing accumulo... On a big cluster, system props can be set in accumulo-env.sh, so it makes sense to me that there would also be a way to set them in a minicluster.
That said, I don't think we actually need it for the SSL changeset quite yet. I added it so that I could test JSSE, but it turns out that doesn't work at all with the ZK/JSSE incompatibility, so if it's controversial we could take it out. But I'll want it back in if ZOOKEEPER-1554 gets addressed and we want to flesh out JSSE support.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > server/src/main/resources/web/flot/jquery.flot.js, line 1560
> > <https://reviews.apache.org/r/14972/diff/1/?file=371891#file371891line1560>
> >
> > why was this change made? What does it have to do w/ ssl?
It has nothing to do with this particular changeset except that I hate seeing errors in my IDE while I'm working on things. I have no problem taking it out of this commit, but I would love if a committer could fix it one way or another.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java, line 62
> > <https://reviews.apache.org/r/14972/diff/1/?file=371859#file371859line62>
> >
> > With authentication via certs isn't this assumption dangerous?
Yeah, when I originally wrote this, I thought the TransportPool was local to the ZKInstance so it seemed like an unnecessary check, but now that I realize it's JVM global, it does seem like it could be a problem. Not a big security risk, since if someone has a cert in the JVM, anyone else in that same JVM could potentially get that same cert, but definitely a bug if we end up using client certs for auth someday. This issue is tracked in ACCUMULO-1729, but I'll deal with it sooner rather than later.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloOutputFormat.java, line 205
> > <https://reviews.apache.org/r/14972/diff/1/?file=371862#file371862line205>
> >
> > I do not see a code change for OutputConfigurator?
> >
> > I am curious what the behavior is when the instance is also set in client config.
> >
OutputConfigurator doesn't need a code change, since the underlying behavior change is implemented by its superclass, ConfiguratorBase.
At the moment, the instanceName parameter overrides any set in the clientConfig (as does the zookeepers connection string). In fact, it looks like those switches in the client config are completely ignored no matter what, and the parameters are required. Probably this should be changed around so that there's a single-argument clientconfig version, and the old interface is just implemented in terms of that. There is a backwards compatibility issue, though...even if the new setters only set the clientconfig argument, getInstance still needs to support the old switches if we want to be able to keep running jobs compiled against 1.5. What's our contract there? I know we're always source code compatible unless there's been a deprecation cycle, but are we also trying to maintain bytecode compatibility? Or do you need to have 1.6 in your client's classpath if you're talking to a 1.6 server?
On Oct. 29, 2013, 3:25 p.m., John Vines wrote:
> > I suggested some test in this comment https://issues.apache.org/jira/browse/ACCUMULO-1009?focusedCommentId=13771990&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13771990
> >
> > Do you know if listscans still show the client IP properly with this change?
Oh yeah, thanks for the reminder. I will add an appropriate test.
I'll also add all the @sinces and other updates discussed above. I'll file a fresh review with the next version, now that I have an account here, so Vines stops getting all the notifications.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by Michael Berman <mb...@sqrrl.com>.
> On Oct. 29, 2013, 3:25 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java, line 161
> > <https://reviews.apache.org/r/14972/diff/1/?file=371870#file371870line161>
> >
> > there is also $ACCUMULO_CONF_DIR (not sure if I have the var name exactly correct)
>
> Michael Berman wrote:
> I was wondering about this... The code that I generalized that from only paid attention to $ACCUMULO_HOME and not the other one, and as I was looking through the codebase it seemed about 50/50 whether a given code path respected $ACCUMULO_CONF_DIR. What is the intention there? Is the plan for both to co-exist (and $ACCUMULO_CONF_DIR overrides $ACCUMULO_HOME/conf)? Should we go through and make sure it's universally supported?
>
> Sean Busbey wrote:
> everything should be using $ACCUMULO_CONF_DIR. if there are places that don't contain it, please file a bug that points to as many of them as possible. If you are adding in new code, please use it.
>
> There are some existing bugs for it not being set correctly in tests.
>
> I believe one of config.sh or bin/accumulo is responsible for handling the fall back to $ACCUMULO_HOME/conf when $ACCUMULO_CONF_DIR doesn't exist.
Filed ACCUMULO-1839 with the few remaining ones I could find. There aren't many; looks like it's been cleaned up more since last I checked.
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
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
>
>
Re: Review Request 14972: ACCUMULO-1009 - add use of SSL for thrift comms
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/14972/#review27675
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
<https://reviews.apache.org/r/14972/#comment53735>
I think I advocated somewhere for removing this method, because its not scaleable for every feature to have a seat at the table in the constructor. The constructor that takes the config object nicely covers this use case.
core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
<https://reviews.apache.org/r/14972/#comment53743>
Probably do not need to add this method. I think most users connect using the instance name. If they really want to connect using the instance id and pass config, can they use the constructor that only takes config?
core/src/main/java/org/apache/accumulo/core/client/impl/ServerClient.java
<https://reviews.apache.org/r/14972/#comment53736>
Communication failures are the norm, this should probably stay as debug?
core/src/main/java/org/apache/accumulo/core/client/impl/ThriftTransportKey.java
<https://reviews.apache.org/r/14972/#comment53737>
With authentication via certs isn't this assumption dangerous?
core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloOutputFormat.java
<https://reviews.apache.org/r/14972/#comment53742>
I do not see a code change for OutputConfigurator?
I am curious what the behavior is when the instance is also set in client config.
core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java
<https://reviews.apache.org/r/14972/#comment53738>
there is also $ACCUMULO_CONF_DIR (not sure if I have the var name exactly correct)
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
<https://reviews.apache.org/r/14972/#comment53741>
add @since tags
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
<https://reviews.apache.org/r/14972/#comment53739>
add @since tags
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
<https://reviews.apache.org/r/14972/#comment53746>
Mini is intended to be user facing and allow user to test their code against Accumulo. We keep exposing methods in the minicluster public API inorder for Accumulo to test Accumulo. I worry that the more we expose in its API the more we will box ourselves in for future changes to mini (like speeding it up some way).
This is a general concern I have, not specific to this change. This change is following the general trend for mini. I have not had time to pursue, but I have wondered if it would be worthwhile to create a mini for internal use and one for external use (with a more minimal API). This may not be worthwhile.
minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java
<https://reviews.apache.org/r/14972/#comment53740>
add @since tags
server/src/main/resources/web/flot/jquery.flot.js
<https://reviews.apache.org/r/14972/#comment53747>
why was this change made? What does it have to do w/ ssl?
test/pom.xml
<https://reviews.apache.org/r/14972/#comment53749>
All export control issues w/ this are resolved?
test/src/test/java/org/apache/accumulo/test/ConditionalWriterTest.java
<https://reviews.apache.org/r/14972/#comment53750>
Whats going on w/ the white space changes in this file? Are you using an incorrect config or was this file checked in w/ incorrect formatting?
I suggested some test in this comment https://issues.apache.org/jira/browse/ACCUMULO-1009?focusedCommentId=13771990&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-13771990
Do you know if listscans still show the client IP properly with this change?
- kturner
On Oct. 26, 2013, 2:36 a.m., John Vines wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/14972/
> -----------------------------------------------------------
>
> (Updated Oct. 26, 2013, 2:36 a.m.)
>
>
> Review request for accumulo.
>
>
> 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
>
>