You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Bill Havanki <bh...@clouderagovt.com> on 2014/03/19 17:44:25 UTC
Review Request 19409: ACCUMULO-2497 - InstanceFactory
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/
-----------------------------------------------------------
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
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On March 19, 2014, 12: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.
>
> Mike Drob wrote:
> 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.
I figure having the exception available just covers our bases. We're also looking at a client API refactoring, so who knows what Instance will be after that?
To satisfy John's wish for adding error codes, I was going to use the commons-lang ContextedRuntimeException, which lets you throw in key(String)/value(Object) pairs. Super keen.
http://commons.apache.org/proper/commons-lang/javadocs/api-3.2.1/org/apache/commons/lang3/exception/ContextedRuntimeException.html
It requires commons-lang 3, but that can co-exist with commons-lang 2.x, which we currently use, without conflict.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37731
-----------------------------------------------------------
On March 19, 2014, 12: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, 12: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Mike Drob <md...@mdrob.com>.
> 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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by John Vines <vi...@apache.org>.
> 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).
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?
- John
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On March 19, 2014, 12: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.
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.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37731
-----------------------------------------------------------
On March 19, 2014, 12: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, 12: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On March 19, 2014, 12:52 p.m., Mike Drob wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java, line 27
> > <https://reviews.apache.org/r/19409/diff/1/?file=528072#file528072line27>
> >
> > Nit: whitespace.
Our code style rules do that. Some sort of bug in Eclipse. I'll trim 'em.
> On March 19, 2014, 12: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.
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).
> On March 19, 2014, 12:52 p.m., Mike Drob wrote:
> > core/src/test/java/org/apache/accumulo/core/client/impl/ZooKeeperInstanceFactoryTest.java, line 23
> > <https://reviews.apache.org/r/19409/diff/1/?file=528074#file528074line23>
> >
> > Please enumerate the imports rather than using wildcards.
OK
> On March 19, 2014, 12:52 p.m., Mike Drob wrote:
> > core/src/test/java/org/apache/accumulo/core/client/impl/ZooKeeperInstanceFactoryTest.java, line 33
> > <https://reviews.apache.org/r/19409/diff/1/?file=528074#file528074line33>
> >
> > createNiceMock takes care of this for you.
I'll do it!
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37731
-----------------------------------------------------------
On March 19, 2014, 12: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, 12: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Sean Busbey <se...@manvsbeard.com>.
> 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?
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.
- Sean
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37731
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java
<https://reviews.apache.org/r/19409/#comment69375>
Nit: whitespace.
core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java
<https://reviews.apache.org/r/19409/#comment69374>
Is this exception class used? If not, it should be removed.
core/src/test/java/org/apache/accumulo/core/client/impl/ZooKeeperInstanceFactoryTest.java
<https://reviews.apache.org/r/19409/#comment69376>
Please enumerate the imports rather than using wildcards.
core/src/test/java/org/apache/accumulo/core/client/impl/ZooKeeperInstanceFactoryTest.java
<https://reviews.apache.org/r/19409/#comment69377>
createNiceMock takes care of this for you.
- Mike Drob
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
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.
Changes
-------
Resolves Mike's issues: whitespace, createNiceMock, no static wildcard imports. Changes InstanceFactoryException to extend commons-lang 3 ContextedRuntimeException to support error codes et al.
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 (updated)
-----
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
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> On March 19, 2014, 3:26 p.m., Sean Busbey wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java, lines 21-23
> > <https://reviews.apache.org/r/19409/diff/1/?file=528072#file528072line21>
> >
> > Depending on the outcome of the discussion about what version this is public or internal use, a note about why this is in the impl package and not public would be a good addition for the javadocs.
>
> Bill Havanki wrote:
> Yes! I forgot I wanted to add that kind of note. Regardless of the outcome, I expect to have this stuff in the public API for 1.7.
Yes! I forgot I wanted to add that kind of note, thank you. Regardless of the outcome, I expect to have this stuff in the public API for 1.7.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37773
-----------------------------------------------------------
On March 19, 2014, 12: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, 12: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37773
-----------------------------------------------------------
On March 19, 2014, 12: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, 12: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Sean Busbey <se...@manvsbeard.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37773
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java
<https://reviews.apache.org/r/19409/#comment69481>
Depending on the outcome of the discussion about what version this is public or internal use, a note about why this is in the impl package and not public would be a good addition for the javadocs.
- Sean Busbey
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Sean Busbey <se...@manvsbeard.com>.
> On March 19, 2014, 6:33 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>
> >
> > The problem here is that client code can never catch an InstanceFactoryException because it's not scoped public. So, in essense, you'd be throwing a RuntimeException with a slightly nicer message.
> >
> > I appreciate the idea of future proofing the code, but in all likelyhood, YAGNI. And since it's an unchecked exception, it is *really* easy to add it in later if we want to.
>
> John Vines wrote:
> Can you please reply to comments instead of creating a new review, it makes it difficult to follow conversations.
Probably Mike is looking at the Diff view and attempting to reply there, which opens new issues.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37758
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Mike Drob <md...@mdrob.com>.
> On March 19, 2014, 6:33 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>
> >
> > The problem here is that client code can never catch an InstanceFactoryException because it's not scoped public. So, in essense, you'd be throwing a RuntimeException with a slightly nicer message.
> >
> > I appreciate the idea of future proofing the code, but in all likelyhood, YAGNI. And since it's an unchecked exception, it is *really* easy to add it in later if we want to.
>
> John Vines wrote:
> Can you please reply to comments instead of creating a new review, it makes it difficult to follow conversations.
>
> Sean Busbey wrote:
> Probably Mike is looking at the Diff view and attempting to reply there, which opens new issues.
Yep.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37758
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by John Vines <vi...@apache.org>.
> On March 19, 2014, 6:33 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>
> >
> > The problem here is that client code can never catch an InstanceFactoryException because it's not scoped public. So, in essense, you'd be throwing a RuntimeException with a slightly nicer message.
> >
> > I appreciate the idea of future proofing the code, but in all likelyhood, YAGNI. And since it's an unchecked exception, it is *really* easy to add it in later if we want to.
Can you please reply to comments instead of creating a new review, it makes it difficult to follow conversations.
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37758
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37758
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java
<https://reviews.apache.org/r/19409/#comment69427>
Yea, known issue, fixed in Eclipse 4.4, whatever that is.
core/src/main/java/org/apache/accumulo/core/client/impl/InstanceFactory.java
<https://reviews.apache.org/r/19409/#comment69426>
The problem here is that client code can never catch an InstanceFactoryException because it's not scoped public. So, in essense, you'd be throwing a RuntimeException with a slightly nicer message.
I appreciate the idea of future proofing the code, but in all likelyhood, YAGNI. And since it's an unchecked exception, it is *really* easy to add it in later if we want to.
- Mike Drob
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Sean Busbey <se...@manvsbeard.com>.
> On March 19, 2014, 4: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?
how would that differ from o.a.a.core.client.impl?
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by John Vines <vi...@apache.org>.
> On March 19, 2014, 4: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?
>
> Bill Havanki wrote:
> Dunno, except that .impl seems permanent in intent. Maybe .experimental? .beta? (We could use Guava's @Beta annotation, actually.) .future? Just brainstorming.
Do we see this eventually becoming part of the actual client API? Is there a reason we would want clients to use this over the existing calls?
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
On March 19, 2014, 9: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, 9: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Josh Elser <jo...@gmail.com>.
> On March 19, 2014, 4: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?
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.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by John Vines <vi...@apache.org>.
> On March 19, 2014, 4: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?
>
> Bill Havanki wrote:
> Dunno, except that .impl seems permanent in intent. Maybe .experimental? .beta? (We could use Guava's @Beta annotation, actually.) .future? Just brainstorming.
>
> John Vines wrote:
> Do we see this eventually becoming part of the actual client API? Is there a reason we would want clients to use this over the existing calls?
>
> Bill Havanki wrote:
> Because a client may want to make more testable code too!
>
> Instance instance = ifactory.getInstance();
> Connector connector = instance.getConnector(principal, token);
> Scanner scanner = connector.createScanner(tableNameParameter, authorizations);
> // ... and so on
>
> The instance, connector, and scanner can all be mock objects in this case, so the client's test can run without a cluster.
So no necessity outside of a testing context? In that case, I see no reason to jam it into 1.6.0
- John
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
On March 19, 2014, 9: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, 9: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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?
>
> Bill Havanki wrote:
> Dunno, except that .impl seems permanent in intent. Maybe .experimental? .beta? (We could use Guava's @Beta annotation, actually.) .future? Just brainstorming.
>
> John Vines wrote:
> Do we see this eventually becoming part of the actual client API? Is there a reason we would want clients to use this over the existing calls?
Because a client may want to make more testable code too!
Instance instance = ifactory.getInstance();
Connector connector = instance.getConnector(principal, token);
Scanner scanner = connector.createScanner(tableNameParameter, authorizations);
// ... and so on
The instance, connector, and scanner can all be mock objects in this case, so the client's test can run without a cluster.
- 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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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.
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?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
On March 19, 2014, 12: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, 12: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Josh Elser <jo...@gmail.com>.
> On March 19, 2014, 4: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?
Or, copying what you had on the ticket: 1.6.0 if it is so decided.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Sean Busbey <se...@manvsbeard.com>.
> On March 19, 2014, 4: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.
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.
- Sean
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Bill Havanki <bh...@clouderagovt.com>.
> 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.
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?
- Bill
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
On March 19, 2014, 12: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, 12: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
>
>
Re: Review Request 19409: ACCUMULO-2497 - InstanceFactory
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/19409/#review37732
-----------------------------------------------------------
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
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
>
>