You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Jenna Huston <je...@gmail.com> on 2014/10/03 19:30:54 UTC
Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs
-----
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by ke...@deenlo.com.
On Oct. 6, 2014, 2:19 p.m., Jenna Huston wrote:
> > Did you plan to change the proxy API and/or create table command in shell?
>
> Jenna Huston wrote:
> I am in the process of testing the new command option in the shell. Can you suggest a test that would be good to look at for testing the new option.
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55498
-----------------------------------------------------------
On Oct. 3, 2014, 5:30 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 5:30 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Christopher Tubbs <ct...@apache.org>.
> On Oct. 6, 2014, 10:19 a.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java, line 200
> > <https://reviews.apache.org/r/26188/diff/1/?file=713519#file713519line200>
> >
> > Is there a benefit to deprecating here if its deprecated in the parent class? I am not sure if its needed, does the deprecated annotation inherit?
It's good practice to deprecate implementing sub-class methods for deprecated interface methods, unless there's a good reason to expect the sub-class to be referenced directly and it still needs the method. Annotations are not inherited, and can lead to API confusion if it's deprecated in an interface, but not in the implementing class.
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55498
-----------------------------------------------------------
On Oct. 3, 2014, 1:30 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 1:30 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
On Oct. 6, 2014, 2:19 p.m., Jenna Huston wrote:
> > Did you plan to change the proxy API and/or create table command in shell?
I am in the process of testing the new command option in the shell. Can you suggest a test that would be good to look at for testing the new option.
- Jenna
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55498
-----------------------------------------------------------
On Oct. 3, 2014, 5:30 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 5:30 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by ke...@deenlo.com.
On Oct. 6, 2014, 2:19 p.m., Jenna Huston wrote:
> > Did you plan to change the proxy API and/or create table command in shell?
>
> Jenna Huston wrote:
> I am in the process of testing the new command option in the shell. Can you suggest a test that would be good to look at for testing the new option.
>
> kturner wrote:
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java
org.apache.accumulo.shell.commands.CloneTableCommand has the ability to set props on the clone being created.
- kturner
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55498
-----------------------------------------------------------
On Oct. 6, 2014, 6:40 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 6:40 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55498
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java
<https://reviews.apache.org/r/26188/#comment95884>
I don't think this method should be deprecated. Its probably the most commonly used create table call. Also its still useful in the case where a user wants default config.
core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95885>
Could init this to DEFAULT_TIME_TYPE instead of null and remove the null check in get.
core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95886>
Could add a check to ensure no null.
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
<https://reviews.apache.org/r/26188/#comment95887>
Is there a benefit to deprecating here if its deprecated in the parent class? I am not sure if its needed, does the deprecated annotation inherit?
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java
<https://reviews.apache.org/r/26188/#comment95888>
Time type is not a property of the table. Its stored in the metadata table. Would need to scan the metadata table for the table and verify the time column value has a prefix of L. Still good to compare props also.
Did you plan to change the proxy API and/or create table command in shell?
- kturner
On Oct. 3, 2014, 5:30 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 5:30 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55576
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95936>
Can use guava Precondition here to check that tt is not null.
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95937>
can also use argument checker to ensure not null.
Could copy the map instead of referencing it.
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95938>
Could return Collections.immuatableMap(properties)
- kturner
On Oct. 6, 2014, 6:40 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 6:40 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55799
-----------------------------------------------------------
Ship it!
One minor fix needed. Some style/whitespace issues still, but it looks good functionally. Thanks for making the changes. Good to go after the last change.
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
<https://reviews.apache.org/r/26188/#comment96181>
Change to org.apache.commons.lang.StringUtils. We have a dependency on commons-lang 2.4. Commons-lang3 is probably on the classpath somehow (likely from Hadoop), but we want to use classes from the libraries we directly depend on most.
- Josh Elser
On Oct. 8, 2014, 3:21 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 3:21 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review56523
-----------------------------------------------------------
Ship it!
Ship It!
- kturner
On Oct. 14, 2014, 1 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2014, 1 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review58491
-----------------------------------------------------------
Ship it!
Thanks for the extra changes. I'm +1
- Josh Elser
On Oct. 14, 2014, 2:32 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2014, 2:32 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 14, 2014, 2:32 p.m.)
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review56524
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
<https://reviews.apache.org/r/26188/#comment96887>
Would be nice to consolidate the duplicate "new NewTableConfiguration().setTimeType(timeType)" once
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
<https://reviews.apache.org/r/26188/#comment96888>
Would be nice to consolidate the duplicate "new NewTableConfiguration().setTimeType(timeType)" once
- Josh Elser
On Oct. 14, 2014, 1 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 14, 2014, 1 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 14, 2014, 1 p.m.)
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review56160
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
<https://reviews.apache.org/r/26188/#comment96479>
Can this method be refactored to call create(String, NewTableConfiguration) ?
- kturner
On Oct. 8, 2014, 6:28 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 8, 2014, 6:28 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 6:28 p.m.)
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 3:21 p.m.)
Review request for accumulo.
Changes
-------
Took out some tracers that I missed.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 8, 2014, 3:05 p.m.)
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Christopher Tubbs <ct...@apache.org>.
> On Oct. 7, 2014, 2:16 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java, line 52
> > <https://reviews.apache.org/r/26188/diff/4/?file=714575#file714575line52>
> >
> > Some javadoc here about how "withoutDefaultIterators" really means "no VersioningIterator" would be good. Actually, will we have other default iterators down the road? Maybe it makes sense to just rename this to be 'withoutVersioningIterator" or similar?
+1 for the javadoc, but regarding the name, I actually thought the opposite. There is no context right now to explain why there should be options to configure one particular iterator, but not the others. The context that should be conveyed is that this particular iterator is on by default, so I think the name should reflect that. The javadoc should definitely highlight which iterators are included in the defaults, though.
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55670
-----------------------------------------------------------
On Oct. 7, 2014, 9:35 a.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 9:35 a.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
> On Oct. 7, 2014, 6:16 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java, line 52
> > <https://reviews.apache.org/r/26188/diff/4/?file=714575#file714575line52>
> >
> > Some javadoc here about how "withoutDefaultIterators" really means "no VersioningIterator" would be good. Actually, will we have other default iterators down the road? Maybe it makes sense to just rename this to be 'withoutVersioningIterator" or similar?
>
> Christopher Tubbs wrote:
> +1 for the javadoc, but regarding the name, I actually thought the opposite. There is no context right now to explain why there should be options to configure one particular iterator, but not the others. The context that should be conveyed is that this particular iterator is on by default, so I think the name should reflect that. The javadoc should definitely highlight which iterators are included in the defaults, though.
>
> Josh Elser wrote:
> My only concern was calling it "default iterators" when there is only one default iterator. If we're future proofing for more default iterators in the future, that's fine. That would reduce the number of methods we eventually need for NewTableConfiguration.
>
> Josh Elser wrote:
> Actually, it's a little inconsistent right now using a boolean to track limitVersion. I think it would make more sense to rely on getProperties in the server instead of both getProperties and getLimitVersion. This then makes limitVersion private. Then, the implementation can perform a merge of the default iterator properties with the user-set properties and get a consistent view of what all properties would be (rather than the boolean + the map).
I think I made all of the changes we talked about yesturday. If I forgot something let me know.
- Jenna
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55670
-----------------------------------------------------------
On Oct. 7, 2014, 1:35 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 1:35 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 7, 2014, 6:16 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java, line 52
> > <https://reviews.apache.org/r/26188/diff/4/?file=714575#file714575line52>
> >
> > Some javadoc here about how "withoutDefaultIterators" really means "no VersioningIterator" would be good. Actually, will we have other default iterators down the road? Maybe it makes sense to just rename this to be 'withoutVersioningIterator" or similar?
>
> Christopher Tubbs wrote:
> +1 for the javadoc, but regarding the name, I actually thought the opposite. There is no context right now to explain why there should be options to configure one particular iterator, but not the others. The context that should be conveyed is that this particular iterator is on by default, so I think the name should reflect that. The javadoc should definitely highlight which iterators are included in the defaults, though.
My only concern was calling it "default iterators" when there is only one default iterator. If we're future proofing for more default iterators in the future, that's fine. That would reduce the number of methods we eventually need for NewTableConfiguration.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55670
-----------------------------------------------------------
On Oct. 7, 2014, 1:35 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 1:35 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Josh Elser <jo...@gmail.com>.
> On Oct. 7, 2014, 6:16 p.m., Josh Elser wrote:
> > core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java, line 52
> > <https://reviews.apache.org/r/26188/diff/4/?file=714575#file714575line52>
> >
> > Some javadoc here about how "withoutDefaultIterators" really means "no VersioningIterator" would be good. Actually, will we have other default iterators down the road? Maybe it makes sense to just rename this to be 'withoutVersioningIterator" or similar?
>
> Christopher Tubbs wrote:
> +1 for the javadoc, but regarding the name, I actually thought the opposite. There is no context right now to explain why there should be options to configure one particular iterator, but not the others. The context that should be conveyed is that this particular iterator is on by default, so I think the name should reflect that. The javadoc should definitely highlight which iterators are included in the defaults, though.
>
> Josh Elser wrote:
> My only concern was calling it "default iterators" when there is only one default iterator. If we're future proofing for more default iterators in the future, that's fine. That would reduce the number of methods we eventually need for NewTableConfiguration.
Actually, it's a little inconsistent right now using a boolean to track limitVersion. I think it would make more sense to rely on getProperties in the server instead of both getProperties and getLimitVersion. This then makes limitVersion private. Then, the implementation can perform a merge of the default iterator properties with the user-set properties and get a consistent view of what all properties would be (rather than the boolean + the map).
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55670
-----------------------------------------------------------
On Oct. 7, 2014, 1:35 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 1:35 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55670
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment96030>
You can use checkNotNull for a bit more simplicity (or if you do another update). Not necessary to change it if there's no other feedback.
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment96032>
Some javadoc here about how "withoutDefaultIterators" really means "no VersioningIterator" would be good. Actually, will we have other default iterators down the road? Maybe it makes sense to just rename this to be 'withoutVersioningIterator" or similar?
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
<https://reviews.apache.org/r/26188/#comment96033>
Whitespace nits
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java
<https://reviews.apache.org/r/26188/#comment96034>
Might be better to use a StringUtils to split this over Java String's split. Hadoop and commons-lang have more performant alternatives
- Josh Elser
On Oct. 7, 2014, 1:35 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 1:35 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55694
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment96071>
This will inadvertently share the same instance of a HashMap. You can just instantiate properties with a new HashMap.
- Josh Elser
On Oct. 7, 2014, 1:35 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 7, 2014, 1:35 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 7, 2014, 1:35 p.m.)
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Christopher Tubbs <ct...@apache.org>.
> On Oct. 6, 2014, 4:35 p.m., kturner wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java, line 241
> > <https://reviews.apache.org/r/26188/diff/3/?file=714180#file714180line241>
> >
> > Needs a @since 1.7.0 javadoc tag
Make sure to add the suggested javadocs to the TableOperations interface, not the TableOperationsImpl.java class. Javadocs are inherited from the interface, so that'd it'd be redundant to add to the implementing class. However, do propagate annotations to sub-classes, as those are not inherited. @Override, and @Deprecated (which is distinct form the javadoc tag @deprecated).
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55575
-----------------------------------------------------------
On Oct. 6, 2014, 2:40 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 2:40 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55575
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95930>
needs a "@since 1.7.0" javadoc tag
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
<https://reviews.apache.org/r/26188/#comment95931>
I copied the following from another method, need something like that in the javadoc that states when it was deprecated and what to use instead
@deprecated since 1.5.0; use {@link #listSplits(String)} instead.
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java
<https://reviews.apache.org/r/26188/#comment95932>
Needs a @since 1.7.0 javadoc tag
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java
<https://reviews.apache.org/r/26188/#comment95935>
There is a constant for the time column. Its nice to use that, makes it easy to find all the code that uses the column.
Could resolve the table name to table id once outside the loop.
- kturner
On Oct. 6, 2014, 6:40 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 6, 2014, 6:40 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
> test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 6, 2014, 6:40 p.m.)
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Jenna Huston <je...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/
-----------------------------------------------------------
(Updated Oct. 6, 2014, 6:27 p.m.)
Review request for accumulo.
Bugs: ACCUMULO-3176
https://issues.apache.org/jira/browse/ACCUMULO-3176
Repository: accumulo
Description
-------
Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
Diffs (updated)
-----
core/src/main/java/org/apache/accumulo/core/client/NewTableConfiguration.java PRE-CREATION
core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af
Diff: https://reviews.apache.org/r/26188/diff/
Testing
-------
New IT, ran unit test and integration tests
Thanks,
Jenna Huston
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Christopher Tubbs <ct...@apache.org>.
> On Oct. 3, 2014, 2:38 p.m., Christopher Tubbs wrote:
> > core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java, lines 46-53
> > <https://reviews.apache.org/r/26188/diff/1/?file=713518#file713518line46>
> >
> > This configuration class needs additional options, and might benefit (in terms of readability, and fluent usage) from builder-pattern naming conventions instead of setter/getter naming conventions.
> >
> > Suggestions below, for discussion:
> >
> > withoutDefaultConstraints();
> > withConstraint(TableConstraint constraint);
> > withIterator(IteratorSetting iterator);
> > withoutDefaultIterators(); // replaces the limit version methods
> > withTimeType(TimeType tt);
> > withAdditionalProperties(Map<String, String> props);
> >
> > // asOffline(); // for future, see ACCUMULO-1904
> >
> > Alternatively, the setter/getter terminology could stay, but with options to remove defaults:
> >
> > removeDefaultConstraints();
> > removeDefaultIterators();
After looking at the current table creation options, it seems that from the above, the only ones needed to carry over from the existing features, are the methods to set the time type and choose to omit the default iterators (I'm in favor of referring to the default iterators generically, rather than have specific methods for the versioning iterator, to prevent the API from getting bloated if we decide to add more default iterators that make sense.)
Everything else I listed here can be added later, as follow-on issues to improve the NewTableConfiguration.
I'm not too stuck on the builder-pattern naming. Standard setters/getters are probably the simplest to deal with for now. We can defer the builder pattern to ACCUMULO-1904.
- Christopher
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55372
-----------------------------------------------------------
On Oct. 3, 2014, 1:30 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 1:30 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>
Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
with user specified initial properties
Posted by Christopher Tubbs <ct...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26188/#review55372
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95732>
This shouldn't be in the impl package if it's intended to be part of the public API.
core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95736>
Needs javadoc
core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java
<https://reviews.apache.org/r/26188/#comment95739>
This configuration class needs additional options, and might benefit (in terms of readability, and fluent usage) from builder-pattern naming conventions instead of setter/getter naming conventions.
Suggestions below, for discussion:
withoutDefaultConstraints();
withConstraint(TableConstraint constraint);
withIterator(IteratorSetting iterator);
withoutDefaultIterators(); // replaces the limit version methods
withTimeType(TimeType tt);
withAdditionalProperties(Map<String, String> props);
// asOffline(); // for future, see ACCUMULO-1904
Alternatively, the setter/getter terminology could stay, but with options to remove defaults:
removeDefaultConstraints();
removeDefaultIterators();
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
<https://reviews.apache.org/r/26188/#comment95733>
Look at the Guava Preconditions class. It has utilities to do this check and throw an IllegalArgumentException with a provided message in a one-liner.
core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java
<https://reviews.apache.org/r/26188/#comment95734>
Another opportunity for Preconditions.
- Christopher Tubbs
On Oct. 3, 2014, 1:30 p.m., Jenna Huston wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/26188/
> -----------------------------------------------------------
>
> (Updated Oct. 3, 2014, 1:30 p.m.)
>
>
> Review request for accumulo.
>
>
> Bugs: ACCUMULO-3176
> https://issues.apache.org/jira/browse/ACCUMULO-3176
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> Gives the ability to add properties to tables before they are initialized. Therefore these properties will take effect before the default tablet is created. We create a NewTableConfiguration class and send that in the create method as opposed to adding another method.
>
>
> Diffs
> -----
>
> core/src/main/java/org/apache/accumulo/core/client/admin/TableOperations.java 97f538d
> core/src/main/java/org/apache/accumulo/core/client/impl/NewTableConfiguration.java PRE-CREATION
> core/src/main/java/org/apache/accumulo/core/client/impl/TableOperationsImpl.java e46b9c9
> core/src/main/java/org/apache/accumulo/core/client/mock/MockAccumulo.java 32dbb28
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTable.java 35cbdd2
> core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java 08750fe
> core/src/test/java/org/apache/accumulo/core/client/impl/TableOperationsHelperTest.java 02838ed
> proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java a778add
> shell/src/main/java/org/apache/accumulo/shell/commands/CreateTableCommand.java 81b39d2
> test/src/test/java/org/apache/accumulo/test/CreateTableWithNewTableConfigIT.java PRE-CREATION
>
> Diff: https://reviews.apache.org/r/26188/diff/
>
>
> Testing
> -------
>
> New IT, ran unit test and integration tests
>
>
> Thanks,
>
> Jenna Huston
>
>