accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Mike Drob" <md...@mdrob.com>
Subject Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Date Wed, 19 Mar 2014 20:03:45 GMT


> On March 19, 2014, 4:52 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java, line
37
> > <https://reviews.apache.org/r/19409/diff/1/?file=528072#file528072line37>
> >
> >     Is this exception class used? If not, it should be removed.
> 
> Bill Havanki wrote:
>     It's not, but I wanted to leave a way for future factory implementations to predictably
throw exceptions, at the factory level of abstraction (Effective Java 2nd ed. Item 61).
> 
> John Vines wrote:
>     I like that. Could we add a basic coding to the exception type to make various error
cases easier to add.
>     
>     And is there a particular reason you're making that exception type extend Runtime
instead of just Exception?
> 
> Sean Busbey wrote:
>     I presume it's because if it was not a RuntimeException it would have to go in the
method signature. Since the class isn't public, it would have to throw Exception, which is
not good.
> 
> Bill Havanki wrote:
>     The InstanceFactoryException class is in fact public because all members of interfaces
are public (JLS 6.6.1, also I had to double-check with javap :) ). I definitely intend for
it to be public.
>     
>     I made it a RuntimeException because I judged that recovering from failing to get
an Instance would be unrecoverable. I could be wrong, and if so, I'm happy to make it checked.
>     
>     John, is there any particular technique for adding a coding that you like? Numeric
code? Enum? I'll check around for any existing patterns for this sort of thing.

Ah. Yes, you are correct, it is public. I'm just not used to seeing members in interfaces.

An Instance is just a container for holding a uuid (or name) and a list of zookeepers, right?
I'm not sure I see how that can fail. Unless there's some sort of lookup that needs to happen
that is susceptible to IOException? Yea, that's probably unrecoverable then.


- Mike


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


On March 19, 2014, 4:44 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, 4:44 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/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 
>   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