accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Michael Berman" <mber...@sqrrl.com>
Subject Re: Review Request 15245: ACCUMULO-1009 - add use of SSL for thrift comms (was https://reviews.apache.org/r/14972/)
Date Thu, 07 Nov 2013 17:59:30 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message