accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bill Havanki" <bhava...@clouderagovt.com>
Subject Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Date Wed, 19 Mar 2014 21:52:22 GMT


> On March 19, 2014, 12:54 p.m., Josh Elser wrote:
> > Oh, I see you're targeting 1.6.1 for this - that explains not wanting to add things
to the public API. Just putting them in a different location to avoid the problem doesn't
seem right either. Shouldn't this go into 1.7 then?
> 
> Josh Elser wrote:
>     Or, copying what you had on the ticket: 1.6.0 if it is so decided.
> 
> Bill Havanki wrote:
>     I really want to add unit tests that take advantage of InstanceFactory into the 1.6
line. Since we're (hopefully?) so close to a 1.6.0 release, I didn't want to complicate it
by putting InstanceFactory, for example, into its logical home in the public API. So, putting
it in an impl subpackage is a cruddy compromise.
>     
>     (This is also why I didn't make a MockInstanceFactory. A good one needs package access
to MockInstance (to get a default file system object), but MockInstance is in the public API.)
>     
>     I agree for 1.7 everything can go in the proper places. For 1.6, I'm at a loss for
a "right" solution. Should I consult Mr. Vines, the 1.6 RM?
> 
> Josh Elser wrote:
>     I wouldn't have a problem with putting this into 1.6.0 (maybe with the experimental
annotation to advertise that it's not some new GA "feature"). If someone has issue with it,
I would guess they'll bring it up here -- otherwise, I wouldn't personally be too concerned.
> 
> Sean Busbey wrote:
>     Our API is confusing enough in terms of expected usage. I'd like to keep this internal
use until the discussion over API refactoring and properly planning for client lifecycle happens.
I don't know if that will be in the 1.7 line or later.
> 
> Bill Havanki wrote:
>     I'm fine with keeping this out of the public API for now. It's quite likely that
we'll find more factories we'd like to add and this same issue will crop up. Perhaps a new
package would be in order, such as o.a.a.core.client.internal?
> 
> Sean Busbey wrote:
>     how would that differ from o.a.a.core.client.impl?

Dunno, except that .impl seems permanent in intent. Maybe .experimental? .beta? (We could
use Guava's @Beta annotation, actually.) .future? Just brainstorming.


- Bill


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------


On March 19, 2014, 5:45 p.m., Bill Havanki wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/19409/
> -----------------------------------------------------------
> 
> (Updated March 19, 2014, 5:45 p.m.)
> 
> 
> Review request for accumulo.
> 
> 
> Bugs: ACCUMULO-2497
>     https://issues.apache.org/jira/browse/ACCUMULO-2497
> 
> 
> Repository: accumulo
> 
> 
> Description
> -------
> 
> To help in testing, this commit adds an InstanceFactory interface with some implementations.
> 
> To avoid changing the public API, some classes are in o.a.a.core.client.impl.
> 
> 
> Diffs
> -----
> 
>   core/pom.xml d56ca2c 
>   core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java PRE-CREATION

>   core/src/main/java/org/apache/accumulo/core/client/impl/ZooKeeperInstanceFactory.java
PRE-CREATION 
>   core/src/test/java/org/apache/accumulo/core/client/impl/ZooKeeperInstanceFactoryTest.java
PRE-CREATION 
>   pom.xml f06380e 
>   server/base/src/main/java/org/apache/accumulo/server/client/HdfsZooInstanceFactory.java
PRE-CREATION 
>   server/base/src/test/java/org/apache/accumulo/server/client/HdfsZooInstanceFactoryTest.java
PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/19409/diff/
> 
> 
> Testing
> -------
> 
> Unit tests pass.
> 
> 
> Thanks,
> 
> Bill Havanki
> 
>


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