Mailing-List: contact dev-help@accumulo.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@accumulo.apache.org
Content-Type: multipart/alternative;
 boundary="===============0525723831875149225=="
MIME-Version: 1.0
Subject: Re: Review Request 26188: ACCUMULO-3176 Add ability to create a table
 with user specified initial properties
From: "Jenna Huston" <jenna.huston22@gmail.com>
To: "Christopher Tubbs" <ctubbsii-rb@apache.org>,
 "accumulo" <dev@accumulo.apache.org>, "Josh Elser" <josh.elser@gmail.com>,
 "Jenna Huston" <jenna.huston22@gmail.com>
Date: Wed, 08 Oct 2014 15:02:10 -0000
Message-ID: <20141008150210.24817.60025@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Jenna Huston" <noreply@reviews.apache.org>
References: <20141007181658.24818.49944@reviews.apache.org>
In-Reply-To: <20141007181658.24818.49944@reviews.apache.org>
Reply-To: "Jenna Huston" <jenna.huston22@gmail.com>

--===============0525723831875149225==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


> 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
> 
>


--===============0525723831875149225==--