> On Nov. 7, 2013, 5:35 p.m., kturner wrote: > > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, > > line 265 > > <https://reviews.apache.org/r/15245/diff/2/?file=378388#file378388line265> > > > > This should be deprecated since a new client side configuration was > > introduced. Having the server side configuration here is just confusing. > > Deprecating it will make it clear that its not intended to be used.
I'm happy to add the @Deprecated annotation, but I firmly believe actually removing references to Instance.getConfiguration(), which is pervasive in the codebase, should be a tracked separate task. I'm not sure we actually need to go through a deprecation cycle, since it seems like there's no value in client code actually using this method...the consensus on the old review and conversation in jira and email seems to be that the existence of this method on Instance was an error from the beginning and should be removed completely. Is it worth adding the annotation in this commit but not doing anything about it (introducing a bunch of deprecation warnings elsewhere in the code)? Or should we just deal with it as part of the "remove Instance.getConfiguration" ticket? > On Nov. 7, 2013, 5:35 p.m., kturner wrote: > > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, > > line 273 > > <https://reviews.apache.org/r/15245/diff/2/?file=378388#file378388line273> > > > > Also need to deprecate. This one is only ever called from a single test, I think, so it's easier to deal with than the getter. I'll try to take care of this one. > On Nov. 7, 2013, 5:35 p.m., kturner wrote: > > core/src/main/java/org/apache/accumulo/core/security/ssl/SslConnectionParams.java, > > line 28 > > <https://reviews.apache.org/r/15245/diff/2/?file=378411#file378411line28> > > > > Is this internal code? Should it be in the public API? If not it > > should go in a different package, like core.impl or something that > > indicates its for internal use only. There are already classes in > > core.security that are part of the API. Good point. I'll move it. > On Nov. 7, 2013, 5:35 p.m., kturner wrote: > > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, > > line 145 > > <https://reviews.apache.org/r/15245/diff/2/?file=378388#file378388line145> > > > > In the old review there was discussion about passing in a generic > > commons configuration object and having javadoc point to > > ClientConfiguration. Yeah, ok, I think I'm convinced that a sufficiently suggestive javadoc will address my discoverability concern. - Michael ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15245/#review28381 ----------------------------------------------------------- On Nov. 6, 2013, 2:57 p.m., Michael Berman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15245/ > ----------------------------------------------------------- > > (Updated Nov. 6, 2013, 2:57 p.m.) > > > Review request for accumulo, Sean Busbey, Christopher Tubbs, John Vines, and > kturner. > > > Repository: accumulo > > > Description > ------- > > (was https://reviews.apache.org/r/14972/ ; created new review created so I > get responses instead of Vines) > > Rebased on 1.6.0-SNAPSHOT's current HEAD (which is pretty close to master), > and feedback from the last review integrated. > > Still need to clean up all the references to newly deprecated ZKInstance > constructors and Input/OutputFormat config setters, but wanted to get this up > for feedback sooner. > > > Diffs > ----- > > core/src/main/java/org/apache/accumulo/core/cli/ClientOpts.java 9247d56 > core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java > a85b72e > > 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/AbstractInputFormat.java > 856936e > > core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloInputFormat.java > cccd7b8 > > core/src/main/java/org/apache/accumulo/core/client/mapred/AccumuloMultiTableInputFormat.java > 61838db > > 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/mapreduce/AbstractInputFormat.java > 626a785 > > core/src/main/java/org/apache/accumulo/core/client/mapreduce/AccumuloInputFormat.java > 9ecae53 > > 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/lib/util/ConfiguratorBase.java > 4f8cdb6 > core/src/main/java/org/apache/accumulo/core/conf/AccumuloConfiguration.java > 0a456ba > core/src/main/java/org/apache/accumulo/core/conf/ClientConfiguration.java > PRE-CREATION > core/src/main/java/org/apache/accumulo/core/conf/Property.java b915ee4 > 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 52e1d04 > core/src/main/java/org/apache/accumulo/core/util/shell/ShellOptionsJC.java > cb1f1c8 > > core/src/test/java/org/apache/accumulo/core/client/mapreduce/lib/util/ConfiguratorBaseTest.java > 62564fa > > 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 > > examples/simple/src/main/java/org/apache/accumulo/examples/simple/filedata/FileDataQuery.java > ecb42c7 > > examples/simple/src/main/java/org/apache/accumulo/examples/simple/mapreduce/TokenFileWordCount.java > 674d793 > > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloCluster.java > 42882bb > > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloConfig.java > bfa7922 > > minicluster/src/main/java/org/apache/accumulo/minicluster/MiniAccumuloInstance.java > 540d7ae > > minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterGCTest.java > 6470ce1 > > minicluster/src/test/java/org/apache/accumulo/minicluster/MiniAccumuloClusterTest.java > 26a1546 > proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java c92f73b > > server/base/src/main/java/org/apache/accumulo/server/cli/ClientOnDefaultTable.java > 53f5ac2 > > server/base/src/main/java/org/apache/accumulo/server/cli/ClientOnRequiredTable.java > e9e9bf1 > server/base/src/main/java/org/apache/accumulo/server/cli/ClientOpts.java > 6f3516a > > server/base/src/main/java/org/apache/accumulo/server/client/BulkImporter.java > 606941d > server/base/src/main/java/org/apache/accumulo/server/util/TServerUtils.java > 8abd104 > test/pom.xml 8e4c152 > test/src/main/java/org/apache/accumulo/test/IMMLGBenchmark.java 948a741 > > test/src/main/java/org/apache/accumulo/test/performance/metadata/MetadataBatchScanTest.java > c6ad74f > > test/src/main/java/org/apache/accumulo/test/performance/thrift/NullTserver.java > 78b2564 > test/src/main/java/org/apache/accumulo/test/randomwalk/State.java ea6d8ed > > test/src/main/java/org/apache/accumulo/test/randomwalk/multitable/CopyTool.java > e104c99 > > test/src/main/java/org/apache/accumulo/test/randomwalk/sequential/MapRedVerifyTool.java > 6c7cc63 > test/src/main/java/org/apache/accumulo/test/scalability/ScaleTest.java > e6ba77b > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 31867f3 > test/src/test/java/org/apache/accumulo/test/functional/AbstractMacIT.java > d24b85b > > test/src/test/java/org/apache/accumulo/test/functional/AccumuloInputFormatIT.java > d38536a > test/src/test/java/org/apache/accumulo/test/functional/BulkIT.java 2fb5827 > test/src/test/java/org/apache/accumulo/test/functional/ConcurrencyIT.java > c3d3160 > > test/src/test/java/org/apache/accumulo/test/functional/ConfigurableMacIT.java > 3f60f1d > test/src/test/java/org/apache/accumulo/test/functional/MapReduceIT.java > 9e42e55 > test/src/test/java/org/apache/accumulo/test/functional/ScannerIT.java > 7913089 > test/src/test/java/org/apache/accumulo/test/functional/ShutdownIT.java > 8d58821 > test/src/test/java/org/apache/accumulo/test/functional/SimpleMacIT.java > 9086f13 > 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/util/CertUtils.java > PRE-CREATION > test/src/test/java/org/apache/accumulo/test/util/CertUtilsTest.java > PRE-CREATION > > Diff: https://reviews.apache.org/r/15245/diff/ > > > Testing > ------- > > > Thanks, > > Michael Berman > >
