You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@accumulo.apache.org by Ben Manes <be...@gmail.com> on 2016/09/15 20:33:04 UTC
Review Request 51933: Improved BlockCache eviction policy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/
-----------------------------------------------------------
Review request for accumulo, Josh Elser and kturner.
Bugs: ACCUMULO-4177
https://issues.apache.org/jira/browse/ACCUMULO-4177
Repository: accumulo
Description
-------
TinyLFU-based BlockCache.
Algorithmic details provided in,
http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
Diffs
-----
assemble/conf/templates/accumulo-site.xml eaaf2b6
core/pom.xml b00a65f
core/src/main/java/org/apache/accumulo/core/conf/Property.java bc1e60e
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 5939d2f
pom.xml 2497daa
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 1523c55
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java b3b02ea
Diff: https://reviews.apache.org/r/51933/diff/
Testing
-------
Basic build
Thanks,
Ben Manes
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/#review149238
-----------------------------------------------------------
Ship it!
You're the man, Ben. I was just going to fix them on commit :). Thanks for the updated patch. I'm going to run a local test and then commit!
- Josh Elser
On Sept. 16, 2016, 6:02 p.m., Ben Manes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51933/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2016, 6:02 p.m.)
>
>
> Review request for accumulo, Josh Elser and kturner.
>
>
> Bugs: ACCUMULO-4177
> https://issues.apache.org/jira/browse/ACCUMULO-4177
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> TinyLFU-based BlockCache.
>
> Algorithmic details provided in,
> http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
>
>
> Diffs
> -----
>
> assemble/pom.xml d0f14a9
> assemble/src/main/assemblies/component.xml 6fc6656
> core/pom.xml 44afddb
> core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java 4dfba68
> core/src/main/java/org/apache/accumulo/core/conf/Property.java ede1c6f
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
> pom.xml 54e4a72
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7751681
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 089bd12
>
> Diff: https://reviews.apache.org/r/51933/diff/
>
>
> Testing
> -------
>
> Basic build
>
>
> Thanks,
>
> Ben Manes
>
>
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Ben Manes <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/
-----------------------------------------------------------
(Updated Sept. 16, 2016, 6:02 p.m.)
Review request for accumulo, Josh Elser and kturner.
Bugs: ACCUMULO-4177
https://issues.apache.org/jira/browse/ACCUMULO-4177
Repository: accumulo
Description
-------
TinyLFU-based BlockCache.
Algorithmic details provided in,
http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
Diffs (updated)
-----
assemble/pom.xml d0f14a9
assemble/src/main/assemblies/component.xml 6fc6656
core/pom.xml 44afddb
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java 4dfba68
core/src/main/java/org/apache/accumulo/core/conf/Property.java ede1c6f
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
pom.xml 54e4a72
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7751681
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 089bd12
Diff: https://reviews.apache.org/r/51933/diff/
Testing
-------
Basic build
Thanks,
Ben Manes
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Josh Elser <jo...@gmail.com>.
> On Sept. 16, 2016, 5:13 p.m., kturner wrote:
> > pom.xml, line 167
> > <https://reviews.apache.org/r/51933/diff/2/?file=1499343#file1499343line167>
> >
> > I don't think this new dep will end up in the binary tarball. I was asking Christopher about this offline and he said the following files need to be modified for this to happen :
> >
> > * add dep to assemble/pom.xml
> > * add dep to assemble/src/main/assemblies/component.xml
Ah, that's a good catch I didn't think about.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/#review149215
-----------------------------------------------------------
On Sept. 16, 2016, 1:15 a.m., Ben Manes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51933/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2016, 1:15 a.m.)
>
>
> Review request for accumulo, Josh Elser and kturner.
>
>
> Bugs: ACCUMULO-4177
> https://issues.apache.org/jira/browse/ACCUMULO-4177
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> TinyLFU-based BlockCache.
>
> Algorithmic details provided in,
> http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
>
>
> Diffs
> -----
>
> core/pom.xml 44afddb
> core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java 4dfba68
> core/src/main/java/org/apache/accumulo/core/conf/Property.java ede1c6f
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 8e35705
> pom.xml 54e4a72
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7751681
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 089bd12
>
> Diff: https://reviews.apache.org/r/51933/diff/
>
>
> Testing
> -------
>
> Basic build
>
>
> Thanks,
>
> Ben Manes
>
>
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by ke...@deenlo.com.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/#review149215
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/conf/Property.java (line 246)
<https://reviews.apache.org/r/51933/#comment216771>
Would be good to document the possible values.
pom.xml (line 167)
<https://reviews.apache.org/r/51933/#comment216769>
I don't think this new dep will end up in the binary tarball. I was asking Christopher about this offline and he said the following files need to be modified for this to happen :
* add dep to assemble/pom.xml
* add dep to assemble/src/main/assemblies/component.xml
I think these changes look good.
- kturner
On Sept. 16, 2016, 1:15 a.m., Ben Manes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51933/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2016, 1:15 a.m.)
>
>
> Review request for accumulo, Josh Elser and kturner.
>
>
> Bugs: ACCUMULO-4177
> https://issues.apache.org/jira/browse/ACCUMULO-4177
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> TinyLFU-based BlockCache.
>
> Algorithmic details provided in,
> http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
>
>
> Diffs
> -----
>
> core/pom.xml 44afddb
> core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java 4dfba68
> core/src/main/java/org/apache/accumulo/core/conf/Property.java ede1c6f
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 8e35705
> pom.xml 54e4a72
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7751681
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 089bd12
>
> Diff: https://reviews.apache.org/r/51933/diff/
>
>
> Testing
> -------
>
> Basic build
>
>
> Thanks,
>
> Ben Manes
>
>
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Josh Elser <jo...@gmail.com>.
> On Sept. 16, 2016, 1:16 a.m., Ben Manes wrote:
> > core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java, line 64
> > <https://reviews.apache.org/r/51933/diff/1/?file=1499136#file1499136line64>
> >
> > Thanks. I called the class CaffeinatedBlockCache in HBase, but when porting thought using the policy name was cleaner. I'll fix the HBase patch as well.
Thanks, Ben! Let me poke Keith over on JIRA. Otherwise, I'm content to merge this in today.
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/#review149160
-----------------------------------------------------------
On Sept. 16, 2016, 1:15 a.m., Ben Manes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51933/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2016, 1:15 a.m.)
>
>
> Review request for accumulo, Josh Elser and kturner.
>
>
> Bugs: ACCUMULO-4177
> https://issues.apache.org/jira/browse/ACCUMULO-4177
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> TinyLFU-based BlockCache.
>
> Algorithmic details provided in,
> http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
>
>
> Diffs
> -----
>
> core/pom.xml 44afddb
> core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java 4dfba68
> core/src/main/java/org/apache/accumulo/core/conf/Property.java ede1c6f
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 8e35705
> pom.xml 54e4a72
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7751681
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 089bd12
>
> Diff: https://reviews.apache.org/r/51933/diff/
>
>
> Testing
> -------
>
> Basic build
>
>
> Thanks,
>
> Ben Manes
>
>
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Ben Manes <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/#review149160
-----------------------------------------------------------
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java (line 64)
<https://reviews.apache.org/r/51933/#comment216691>
Thanks. I called the class CaffeinatedBlockCache in HBase, but when porting thought using the policy name was cleaner. I'll fix the HBase patch as well.
- Ben Manes
On Sept. 16, 2016, 1:15 a.m., Ben Manes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51933/
> -----------------------------------------------------------
>
> (Updated Sept. 16, 2016, 1:15 a.m.)
>
>
> Review request for accumulo, Josh Elser and kturner.
>
>
> Bugs: ACCUMULO-4177
> https://issues.apache.org/jira/browse/ACCUMULO-4177
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> TinyLFU-based BlockCache.
>
> Algorithmic details provided in,
> http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
>
>
> Diffs
> -----
>
> core/pom.xml 44afddb
> core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java 4dfba68
> core/src/main/java/org/apache/accumulo/core/conf/Property.java ede1c6f
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 8e35705
> pom.xml 54e4a72
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7751681
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 089bd12
>
> Diff: https://reviews.apache.org/r/51933/diff/
>
>
> Testing
> -------
>
> Basic build
>
>
> Thanks,
>
> Ben Manes
>
>
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Ben Manes <be...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/
-----------------------------------------------------------
(Updated Sept. 16, 2016, 1:15 a.m.)
Review request for accumulo, Josh Elser and kturner.
Bugs: ACCUMULO-4177
https://issues.apache.org/jira/browse/ACCUMULO-4177
Repository: accumulo
Description
-------
TinyLFU-based BlockCache.
Algorithmic details provided in,
http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
Diffs (updated)
-----
core/pom.xml 44afddb
core/src/main/java/org/apache/accumulo/core/client/rfile/RFileScanner.java 4dfba68
core/src/main/java/org/apache/accumulo/core/conf/Property.java ede1c6f
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 8e35705
pom.xml 54e4a72
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 7751681
server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java 089bd12
Diff: https://reviews.apache.org/r/51933/diff/
Testing
-------
Basic build
Thanks,
Ben Manes
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Josh Elser <jo...@gmail.com>.
> On Sept. 15, 2016, 8:48 p.m., Josh Elser wrote:
> > A couple of extremely minor things. In general, this looks really great.
Also, checked out your licensing on Caffeine and it doesn't look like we need to anything else here in Accumulo to add it (yay).
- Josh
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/#review149122
-----------------------------------------------------------
On Sept. 15, 2016, 8:33 p.m., Ben Manes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51933/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2016, 8:33 p.m.)
>
>
> Review request for accumulo, Josh Elser and kturner.
>
>
> Bugs: ACCUMULO-4177
> https://issues.apache.org/jira/browse/ACCUMULO-4177
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> TinyLFU-based BlockCache.
>
> Algorithmic details provided in,
> http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
>
>
> Diffs
> -----
>
> assemble/conf/templates/accumulo-site.xml eaaf2b6
> core/pom.xml b00a65f
> core/src/main/java/org/apache/accumulo/core/conf/Property.java bc1e60e
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 5939d2f
> pom.xml 2497daa
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 1523c55
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java b3b02ea
>
> Diff: https://reviews.apache.org/r/51933/diff/
>
>
> Testing
> -------
>
> Basic build
>
>
> Thanks,
>
> Ben Manes
>
>
Re: Review Request 51933: Improved BlockCache eviction policy
Posted by Josh Elser <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51933/#review149122
-----------------------------------------------------------
A couple of extremely minor things. In general, this looks really great.
assemble/conf/templates/accumulo-site.xml (line 66)
<https://reviews.apache.org/r/51933/#comment216643>
I would remove this since it's the default. Users wouldn't have to do set this on their own in most cases.
core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java (line 64)
<https://reviews.apache.org/r/51933/#comment216642>
s/CaffeinatedBlockCacheStatsExecutor/TinyLfuBlockCacheStatsExecutor/ for naming consistency?
minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java (line 141)
<https://reviews.apache.org/r/51933/#comment216644>
Same here, if it's the default, don't need to explicitly set it.
- Josh Elser
On Sept. 15, 2016, 8:33 p.m., Ben Manes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51933/
> -----------------------------------------------------------
>
> (Updated Sept. 15, 2016, 8:33 p.m.)
>
>
> Review request for accumulo, Josh Elser and kturner.
>
>
> Bugs: ACCUMULO-4177
> https://issues.apache.org/jira/browse/ACCUMULO-4177
>
>
> Repository: accumulo
>
>
> Description
> -------
>
> TinyLFU-based BlockCache.
>
> Algorithmic details provided in,
> http://highscalability.com/blog/2016/1/25/design-of-a-modern-cache.html
>
>
> Diffs
> -----
>
> assemble/conf/templates/accumulo-site.xml eaaf2b6
> core/pom.xml b00a65f
> core/src/main/java/org/apache/accumulo/core/conf/Property.java bc1e60e
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/BlockCache.java 094782d
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/LruBlockCache.java 1beaccb
> core/src/main/java/org/apache/accumulo/core/file/blockfile/cache/TinyLfuBlockCache.java PRE-CREATION
> minicluster/src/main/java/org/apache/accumulo/minicluster/impl/MiniAccumuloConfigImpl.java 5939d2f
> pom.xml 2497daa
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServer.java 1523c55
> server/tserver/src/main/java/org/apache/accumulo/tserver/TabletServerResourceManager.java b3b02ea
>
> Diff: https://reviews.apache.org/r/51933/diff/
>
>
> Testing
> -------
>
> Basic build
>
>
> Thanks,
>
> Ben Manes
>
>