----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26530/#review56431 -----------------------------------------------------------
Looks pretty good overall. Only major concern is the use of null, the rest are minor. core/src/main/java/org/apache/accumulo/core/client/impl/CompactionStrategyConfigUtil.java <https://reviews.apache.org/r/26530/#comment96739> 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. core/src/main/java/org/apache/accumulo/core/client/mock/MockTableOperations.java <https://reviews.apache.org/r/26530/#comment96730> A message that says specifically that MockAccumulo can't use iterators would be nice since the implementation previously silently ignored them. server/base/src/main/java/org/apache/accumulo/server/master/tableOps/UserCompactionConfig.java <https://reviews.apache.org/r/26530/#comment96731> Would it be good to use ByteBuffer instead of, or in addition to, the byte[] arguments? I know we had some talks previously about trying to move towards ByteBuffer, not sure if it makes sense here. server/master/src/main/java/org/apache/accumulo/master/tableOps/CompactRange.java <https://reviews.apache.org/r/26530/#comment96732> This condition strikes me a bit odd -- in the positive, we set config, but in the negative, we set iterators to null. A comment would help. server/tserver/src/main/java/org/apache/accumulo/tserver/compaction/SizeLimitCompactionStrategy.java <https://reviews.apache.org/r/26530/#comment96746> Would be nice to have tests added for the SizeLImitCompactionStrategy since that was the original use case you provided for implementing these changes. server/tserver/src/main/java/org/apache/accumulo/tserver/tablet/Tablet.java <https://reviews.apache.org/r/26530/#comment96735> Obligatory TODO reminder (or remove the comment if thoroughly tested) test/src/test/java/org/apache/accumulo/proxy/SimpleProxyIT.java <https://reviews.apache.org/r/26530/#comment96740> whitespace in this method test/src/test/java/org/apache/accumulo/proxy/SimpleProxyIT.java <https://reviews.apache.org/r/26530/#comment96741> Make sure this jar doesn't add any new RAT warnings. Might have to update excludes in pom. test/src/test/java/org/apache/accumulo/proxy/SimpleProxyIT.java <https://reviews.apache.org/r/26530/#comment96744> Comments please. What does EfgCompactionStrat actually do? If this test started failing, how can I debug it? - Josh Elser 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 > >
