> On Oct. 13, 2014, 8:05 p.m., Josh Elser wrote: > > core/src/main/java/org/apache/accumulo/core/client/impl/CompactionStrategyConfigUtil.java, > > line 79 > > <https://reviews.apache.org/r/26530/diff/1/?file=717092#file717092line79> > > > > I'm not a big fan of using null to denote that there was no compaction > > strategy provided. IMO, a better default would be a compaction strategy > > which chooses all files. This prevents you from pushing that logic down to > > Tablet and adding in extra conditional branches as to whether or not the > > CompactionStrategy is null.
> a better default would be a compaction strategy which chooses all files. I like that, I'll give it a try. > On Oct. 13, 2014, 8:05 p.m., Josh Elser wrote: > > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/SizeLimitCompactionStrategy.java, > > line 31 > > <https://reviews.apache.org/r/26530/diff/1/?file=717107#file717107line31> > > > > Would be nice to have tests added for the SizeLImitCompactionStrategy > > since that was the original use case you provided for implementing these > > changes. Good idea. Some of the test look at filenames, but I don't think any of them consider the file size info. - kturner ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26530/#review56431 ----------------------------------------------------------- On Oct. 9, 2014, 10:59 p.m., kturner wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26530/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2014, 10:59 p.m.) > > > Review request for accumulo. > > > Bugs: ACCUMULO-1798 > https://issues.apache.org/jira/browse/ACCUMULO-1798 > > > Repository: accumulo > > > Description > ------- > > This patch generated w/o thrift using the following command. > > git diff HEAD~1 -- core server shell test proxy/src/main/thrift/proxy.thrift > proxy/src/main/java/org/apache/accumulo/proxy/ProxyServer.java > > The issue has the full patch. > > > Diffs > ----- > > > core/src/main/java/org/apache/accumulo/core/client/admin/CompactionConfig.java > PRE-CREATION > > core/src/main/java/org/apache/accumulo/core/client/admin/CompactionStrategyConfig.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/CompactionStrategyConfigUtil.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/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 > proxy/src/main/thrift/proxy.thrift fbd9c52 > > server/base/src/main/java/org/apache/accumulo/server/master/tableOps/CompactionIterators.java > 4f5bf42 > > server/base/src/main/java/org/apache/accumulo/server/master/tableOps/UserCompactionConfig.java > PRE-CREATION > > server/master/src/main/java/org/apache/accumulo/master/FateServiceHandler.java > 41ca911 > > server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java > dedfb97 > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java > 2e1eb2c > > server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java > 4855a4f > > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/CompactionPlan.java > 6f69fb0 > > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/CompactionStrategy.java > 7bc1a80 > > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/DefaultCompactionStrategy.java > 8b03d17 > > server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/SizeLimitCompactionStrategy.java > 478939a > server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java > 0194778 > shell/src/main/java/org/apache/accumulo/shell/commands/CompactCommand.java > 80dd9ba > test/src/test/java/org/apache/accumulo/proxy/SimpleProxyIT.java 6b4bcfb > test/src/test/java/org/apache/accumulo/test/ShellServerIT.java 5a068af > test/src/test/java/org/apache/accumulo/test/UserCompactionStrategyIT.java > PRE-CREATION > > test/src/test/java/org/apache/accumulo/test/functional/FunctionalTestUtils.java > 1246efe > > Diff: https://reviews.apache.org/r/26530/diff/ > > > Testing > ------- > > mvn package so far > > > Thanks, > > kturner > >
