accumulo-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ke...@deenlo.com
Subject Re: Review Request 26530: ACCUMULO-1798 Add compaction strategy to user compactions
Date Tue, 14 Oct 2014 14:00:32 GMT


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


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