accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Josh Elser" <josh.el...@gmail.com>
Subject Re: Review Request 26530: ACCUMULO-1798 Add compaction strategy to user compactions
Date Mon, 13 Oct 2014 20:05:33 GMT

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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message