Home | About | Sematext search-lucene.com search-hadoop.com
 Search Hadoop and all its subprojects:

Switch to Threaded View
Accumulo, mail # dev - Re: Review Request 15245: ACCUMULO-1009 - add use of SSL for thrift comms (was https://reviews.apache.org/r/14972/)


Copy link to this message
-
Re: Review Request 15245: ACCUMULO-1009 - add use of SSL for thrift comms (was https://reviews.apache.org/r/14972/)
Michael Berman 2013-11-07, 19:31


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

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