You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Mike Drob <md...@mdrob.com> on 2013/12/20 22:52:55 UTC
Review Request 16427: ACCUMULO-2076 add Instance builder based on classpath
resources
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-2076
https://issues.apache.org/jira/browse/ACCUMULO-2076
Repository: accumulo
Description
-------
Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
Diffs
-----
src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java fcadd384cc150b8422c3f5c33970c26efe142132
Diff: https://reviews.apache.org/r/16427/diff/
Testing
-------
None, yet. :)
Thanks,
Mike Drob
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/#review30877
-----------------------------------------------------------
Wait, I just noticed that you have this RB marked as 1.4.5-SNAPSHOT (and your javadoc has @since 1.4.5, etc) but your original ticket is marked for 1.7.0. I do not think this should pushed back to 1.4.5 or 1.5.1. What is your intent?
Also, I was just talking to Bill Slacum about creating Instances earlier by chance. Have you considered expanding this to also support HdfsZooInstance and MiniAccumuloInstance? This would also give us flexibility in (if we move this direction) to having threadsafe and non-threadsafe variants of the aforementioned Instances (also moving towards a fluent interface like Bill Havanki mentioned).
- Josh Elser
On Dec. 26, 2013, 6:33 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16427/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2013, 6:33 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2076
> https://issues.apache.org/jira/browse/ACCUMULO-2076
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/ClasspathZooKeeperInstanceFactory.java PRE-CREATION
> src/core/src/main/java/org/apache/accumulo/core/client/InstanceFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16427/diff/
>
>
> Testing
> -------
>
> None, yet. :)
>
>
> Thanks,
>
> Mike Drob
>
>
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/#review31036
-----------------------------------------------------------
src/core/src/main/java/org/apache/accumulo/core/client/ClasspathZooKeeperInstanceFactory.java
<https://reviews.apache.org/r/16427/#comment59383>
Please see the comment I just made in jira. I think adding this when ClientConfiguration was added in 1.6.0 is very confusing from the the users perspective. Provides slightly different ways to accomplish the same goal.
- kturner
On Dec. 26, 2013, 6:33 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16427/
> -----------------------------------------------------------
>
> (Updated Dec. 26, 2013, 6:33 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2076
> https://issues.apache.org/jira/browse/ACCUMULO-2076
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/ClasspathZooKeeperInstanceFactory.java PRE-CREATION
> src/core/src/main/java/org/apache/accumulo/core/client/InstanceFactory.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/16427/diff/
>
>
> Testing
> -------
>
> None, yet. :)
>
>
> Thanks,
>
> Mike Drob
>
>
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/
-----------------------------------------------------------
(Updated Dec. 26, 2013, 6:33 p.m.)
Review request for accumulo.
Changes
-------
Whitespace errors.
Bugs: ACCUMULO-2076
https://issues.apache.org/jira/browse/ACCUMULO-2076
Repository: accumulo
Description
-------
Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
Diffs (updated)
-----
src/core/src/main/java/org/apache/accumulo/core/client/ClasspathZooKeeperInstanceFactory.java PRE-CREATION
src/core/src/main/java/org/apache/accumulo/core/client/InstanceFactory.java PRE-CREATION
Diff: https://reviews.apache.org/r/16427/diff/
Testing
-------
None, yet. :)
Thanks,
Mike Drob
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by Mike Drob <md...@mdrob.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/
-----------------------------------------------------------
(Updated Dec. 26, 2013, 6:29 p.m.)
Review request for accumulo.
Changes
-------
Incorporated Josh's and Bill's suggestions.
Bugs: ACCUMULO-2076
https://issues.apache.org/jira/browse/ACCUMULO-2076
Repository: accumulo
Description
-------
Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
Diffs (updated)
-----
src/core/src/main/java/org/apache/accumulo/core/client/ClasspathZooKeeperInstanceFactory.java PRE-CREATION
src/core/src/main/java/org/apache/accumulo/core/client/InstanceFactory.java PRE-CREATION
src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java fcadd384cc150b8422c3f5c33970c26efe142132
Diff: https://reviews.apache.org/r/16427/diff/
Testing
-------
None, yet. :)
Thanks,
Mike Drob
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by Mike Drob <md...@mdrob.com>.
> On Dec. 22, 2013, 5:51 p.m., Bill Havanki wrote:
> > Instead of creating a static method on ZooKeeperInstance, I'd prefer to see a ZooKeeperInstanceFactory object with a non-static version of the method in it. Benefits: makes it easier to mock instance creation for testing; allows for alternative implementations if they all implement the same interface (e.g., InstanceFactory); encourages easier-to-use API, like a fluent interface or merely setting named fields, as the number of parameters grows.
> >
> > ZooKeeperInstance zki = new ClasspathZooKeeperInstanceFactory().siteConf("accumulo-site.xml").getInstance();
Done.
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/#review30800
-----------------------------------------------------------
On Dec. 20, 2013, 9:52 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16427/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2013, 9:52 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2076
> https://issues.apache.org/jira/browse/ACCUMULO-2076
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java fcadd384cc150b8422c3f5c33970c26efe142132
>
> Diff: https://reviews.apache.org/r/16427/diff/
>
>
> Testing
> -------
>
> None, yet. :)
>
>
> Thanks,
>
> Mike Drob
>
>
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by Bill Havanki <bh...@clouderagovt.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/#review30800
-----------------------------------------------------------
Instead of creating a static method on ZooKeeperInstance, I'd prefer to see a ZooKeeperInstanceFactory object with a non-static version of the method in it. Benefits: makes it easier to mock instance creation for testing; allows for alternative implementations if they all implement the same interface (e.g., InstanceFactory); encourages easier-to-use API, like a fluent interface or merely setting named fields, as the number of parameters grows.
ZooKeeperInstance zki = new ClasspathZooKeeperInstanceFactory().siteConf("accumulo-site.xml").getInstance();
- Bill Havanki
On Dec. 20, 2013, 4:52 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16427/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2013, 4:52 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2076
> https://issues.apache.org/jira/browse/ACCUMULO-2076
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java fcadd384cc150b8422c3f5c33970c26efe142132
>
> Diff: https://reviews.apache.org/r/16427/diff/
>
>
> Testing
> -------
>
> None, yet. :)
>
>
> Thanks,
>
> Mike Drob
>
>
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by Mike Drob <md...@mdrob.com>.
> On Dec. 21, 2013, 12:18 a.m., Josh Elser wrote:
> > src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java, line 234
> > <https://reviews.apache.org/r/16427/diff/1/?file=401735#file401735line234>
> >
> > Not sure if there's something in ZooKeeperInstance that you can use, but it's excessive to take the instance ID, to find the instance name. Perhaps you can add a protected constructor that you can provide the instance name and ID since you know you calculated them correctly.
Turns out, there is! How did I never see ZooKeeperInstance(UUID, String) consturctor before?
- Mike
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/#review30769
-----------------------------------------------------------
On Dec. 20, 2013, 9:52 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16427/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2013, 9:52 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2076
> https://issues.apache.org/jira/browse/ACCUMULO-2076
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java fcadd384cc150b8422c3f5c33970c26efe142132
>
> Diff: https://reviews.apache.org/r/16427/diff/
>
>
> Testing
> -------
>
> None, yet. :)
>
>
> Thanks,
>
> Mike Drob
>
>
Re: Review Request 16427: ACCUMULO-2076 add Instance builder based on
classpath resources
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/16427/#review30769
-----------------------------------------------------------
src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java
<https://reviews.apache.org/r/16427/#comment58918>
Not sure if there's something in ZooKeeperInstance that you can use, but it's excessive to take the instance ID, to find the instance name. Perhaps you can add a protected constructor that you can provide the instance name and ID since you know you calculated them correctly.
- Josh Elser
On Dec. 20, 2013, 9:52 p.m., Mike Drob wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16427/
> -----------------------------------------------------------
>
> (Updated Dec. 20, 2013, 9:52 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-2076
> https://issues.apache.org/jira/browse/ACCUMULO-2076
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Writing client code for retrieving zookeeper and instance information is both tedious and error prone. It would be great to remove this boiler-plate burden from users.
>
>
> Diffs
> -----
>
> src/core/src/main/java/org/apache/accumulo/core/client/ZooKeeperInstance.java fcadd384cc150b8422c3f5c33970c26efe142132
>
> Diff: https://reviews.apache.org/r/16427/diff/
>
>
> Testing
> -------
>
> None, yet. :)
>
>
> Thanks,
>
> Mike Drob
>
>