Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Received-SPF: pass (nike.apache.org: local policy includes SPF record at
 spf.trusted-forwarder.org)
MIME-Version: 1.0
In-Reply-To: <20131107190855.30022.70120@reviews.apache.org>
References: <20131107173512.30021.36184@reviews.apache.org>
 <20131107190855.30022.70120@reviews.apache.org>
From: Michael Berman <mberman@sqrrl.com>
Date: Thu, 7 Nov 2013 14:16:50 -0500
Message-ID: 
 <CAA+ACqdXPW=KEdXtZKmu7o8KJ_R7eZOyPT0wFhL7_uouCEHMCQ@mail.gmail.com>
Subject: Re: Review Request 15245: ACCUMULO-1009 - add use of SSL for thrift
 comms (was https://reviews.apache.org/r/14972/)
To: dev <dev@accumulo.apache.org>, John Vines <vines@apache.org>
Cc: Christopher Tubbs <ctubbsii@apache.org>,
 Sean Busbey <sean@manvsbeard.com>, keith@deenlo.com,
	accumulo <accumulo-dev@incubator.apache.org>
Content-Type: multipart/alternative; boundary=001a1133458ed6a77c04ea9b1c90

--001a1133458ed6a77c04ea9b1c90
Content-Type: text/plain; charset=ISO-8859-1

I can definitely see an argument for removing
setConfiguration(AccumuloConfiguration) as part of this change, since it's
not clear at all how that interacts (or should interact) with the client
configuration passed into the constructor.  But like John, I also don't see
why changing Instance.getConfiguration() should be tied to this change.  It
existed and has been considered a problem since before this change, and
it's not clear to me that this change makes it any worse.


On Thu, Nov 7, 2013 at 2:08 PM, John Vines <vines@apache.org> wrote:

>
>
> > 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.
>
> I don't see why that change needs to be a blocker for this patch
>
>
> - John
>
>
> -----------------------------------------------------------
> 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
> >
> >
>
>

--001a1133458ed6a77c04ea9b1c90--