> 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. > > Michael Berman wrote: > 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? > > kturner wrote: > I agree, it should be done as a separate ticket. I am mostly concerned > about making a coherent set of API changes for 1.6. So if its done in a > separate ticket, I think that ticket should also go in for 1.6 if this does. > > John Vines wrote: > I don't see why that change needs to be a blocker for this patch > > kturner wrote: > Its not a blocker. If this patch goes in I would like to see those > methods deprecated. I can do it if no one else is interested. > > Michael Berman wrote: > What's the policy for introducing deprecated references into our own > code? getConfiguration is called in 34 places, from core, gc, master, and > server-base. If we deprecate the method, can we still reference it > internally? Can we just suppress the warnings? > > kturner wrote: > We usually remove our usage of deprecated APIs when we deprecate them. > Otherwise it just become technical debt. > > Michael Berman wrote: > Yeah, that's how I think it should work...but in this case it seems like > a big change for this late in the process (which is why my reflex is to not > even deprecate it for 1.6.0, and attack this issue separately).
LEt me look into a bit. I know I want it gone. I have not looked into what it would take. - kturner ----------------------------------------------------------- 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 > >
