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