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