You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2019/03/18 16:35:18 UTC
[kudu-CR] [util] added cache with FIFO eviction policy
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12777
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
WIP: the eviction test for the FIFO cache fails. It seems the cache
has a bug in memory accounting or I'm missing something.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 295 insertions(+), 152 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/1
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 6:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12777/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12777/6//COMMIT_MSG@10
PS6, Line 10:
> nit: extra space.
Done
http://gerrit.cloudera.org:8080/#/c/12777/6/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/12777/6/src/kudu/util/cache.h@15
PS6, Line 15: // TODO: this is pretty lock-heavy. Would be good to sub out something
> warning: missing username/bug in TODO [google-readability-todo]
Done
http://gerrit.cloudera.org:8080/#/c/12777/6/src/kudu/util/cache.h@43
PS6, Line 43: accomodate
> nit: accommodate
Done
http://gerrit.cloudera.org:8080/#/c/12777/6/src/kudu/util/cache.h@223
PS6, Line 223: Cache::MemoryType mem_type
> Can you comment on how is 'mem_type' is used?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 20 Mar 2019 01:06:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 20 Mar 2019 16:10:33 +0000
Gerrit-HasComments: No
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#3).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit tests are updated as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 447 insertions(+), 194 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/3
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#9).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit test is added as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/cfile/block_cache.cc
M src/kudu/codegen/code_cache.cc
M src/kudu/util/cache-bench.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
7 files changed, 274 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/9
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc@170
PS4, Line 170:
:
: void SetUp() override {
> Done except for CacheComposition --> ShardingPolicy. The reason I left it
Ah, I wasn't advocating for exposing stuff from cache.{cc,h}, but just to rename some of these enums so they didn't sound so similar.
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache-test.cc@331
PS8, Line 331: TEST_F(FIFOCacheTest, EvictionPolicy) {
As written I _think_ this test could also pass if it used LRU eviction. You may want to add some "negative" test to it (i.e. do a Lookup of something or some range inserted in the "first data chunk" and show that the Lookup does not cause the looked up elements to be evicted any differently).
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache-test.cc@355
PS8, Line 355: // to accomodate the elements from the second chunk.
accommodate
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache.h@222
PS8, Line 222: (as of now, FIFO and LRU)
Nit: not necessary; it's obvious below.
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache.h@228
PS8, Line 228: Cache* NewFIFOCache(Cache::MemoryType mem_type,
: size_t capacity,
: const std::string& id);
Should note that this type of cache doesn't support the NVM memory type.
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 20 Mar 2019 04:09:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 4:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/12777/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12777/4//COMMIT_MSG@11
PS4, Line 11: TTL cache
> Curious to see how this is going to work. Sounds like maintaining/checking
Yep, the idea is to add a wrapper (something like file_cache.{h,cc}) that will deal with checking TTL. I'm planning to post that patch soon.
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
PS4:
> Similarly, the changes in this test would be clearer if there was one patch
Done
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc@38
PS4, Line 38: using std::make_tuple;
> warning: using decl 'make_tuple' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc@170
PS4, Line 170: CacheType,
: CacheFlavor,
: CacheComposition>>
> Nit: these all sound kind of similar. Maybe change CacheFlavor to EvictionP
Done except for CacheComposition --> ShardingPolicy. The reason I left it as is because I didn't want to expose the cache composition/sharding policy out of the cache, and I wanted to keep the CacheComposition as a test-only thing. If you really prefer it to be named ShardingPolicy (but introduced only in this test), let me know.
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc@326
PS4, Line 326: , FIFOCacheTest, ::testing::Values(CacheComposition::MultiShard,
> Can you put in a name here?
Done
http://gerrit.cloudera.org:8080/#/c/12777/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/12777/2/src/kudu/util/cache.cc@232
PS2, Line 232: Cache::Handle* Insert(RLHandle* handle, Cache::EvictionCallback* eviction_callback);
> warning: function 'kudu::(anonymous namespace)::CacheShard::Insert' has a d
Done
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc@414
PS4, Line 414: RL_Remove(e);
: RL_Append(e);
> AFAICT, this is the only substantive difference between the two caches, rig
Yes, that's the only difference here, in essence. Added AfterLookup().
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc@551
PS4, Line 551: gscoped_ptr<CacheShard<policy>> shard(
> Can switch this to unique_ptr?
Done
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc@593
PS4, Line 593: for (CacheShard<policy>* cache : shards_) {
> auto would be good here.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 19 Mar 2019 23:54:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#6).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit tests are updated as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 190 insertions(+), 38 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/6
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/12777/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12777/6//COMMIT_MSG@10
PS6, Line 10:
nit: extra space.
http://gerrit.cloudera.org:8080/#/c/12777/6/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/12777/6/src/kudu/util/cache.h@43
PS6, Line 43: accomodate
nit: accommodate
http://gerrit.cloudera.org:8080/#/c/12777/6/src/kudu/util/cache.h@223
PS6, Line 223: Cache::MemoryType mem_type
Can you comment on how is 'mem_type' is used?
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 6
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 20 Mar 2019 00:40:11 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#7).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit tests are updated as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 196 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/7
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 7
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#4).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit tests are updated as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 447 insertions(+), 196 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/4
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit test is added as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Reviewed-on: http://gerrit.cloudera.org:8080/12777
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/cfile/block_cache.cc
M src/kudu/codegen/code_cache.cc
M src/kudu/util/cache-bench.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache.cc
7 files changed, 274 insertions(+), 71 deletions(-)
Approvals:
Alexey Serbin: Verified
Adar Dembo: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 10
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#8).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit tests are updated as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 198 insertions(+), 39 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/8
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 8
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12777/4//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12777/4//COMMIT_MSG@11
PS4, Line 11: TTL cache
Curious to see how this is going to work. Sounds like maintaining/checking the TTL will be the cache client's responsibility rather than baked into the cache itself? That seems like a reasonable middle ground while the privilege cache is the only such TTL cache.
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
PS4:
Similarly, the changes in this test would be clearer if there was one patch for the parameterization, cleanup, and code motion, and another patch to introduce the new tests.
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc@170
PS4, Line 170: CacheType,
: CacheFlavor,
: CacheComposition>>
Nit: these all sound kind of similar. Maybe change CacheFlavor to EvictionPolicy and CacheComposition to ShardingPolicy?
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc@326
PS4, Line 326: , FIFOCacheTest, ::testing::Values(CacheComposition::MultiShard,
Can you put in a name here?
Below too.
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc@414
PS4, Line 414: RL_Remove(e);
: RL_Append(e);
AFAICT, this is the only substantive difference between the two caches, right? Any way to share code for Lookup() and have the only difference between the two caches be whether we move looked up elements to the front? Possibly by defining a AfterLookup() private function that either calls these two (LRU) or does nothing (FIFO)?
BTW, would have been better to do as much of the cleanup and renaming in a separate patch so we can focus on the juicy parts here.
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc@551
PS4, Line 551: gscoped_ptr<CacheShard<policy>> shard(
Can switch this to unique_ptr?
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache.cc@593
PS4, Line 593: for (CacheShard<policy>* cache : shards_) {
auto would be good here.
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 19 Mar 2019 04:31:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#2).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
WIP: the eviction test for the FIFO cache fails. It seems the cache
has a bug in memory accounting or I'm missing something.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 280 insertions(+), 137 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/2
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12777/4/src/kudu/util/cache-test.cc@170
PS4, Line 170: CacheType,
: CacheFlavor,
: CacheComposition>>
> Ah, I wasn't advocating for exposing stuff from cache.{cc,h}, but just to r
I see. All right, I renamed CacheComposition --> ShardingPolicy after reading this comment :)
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache-test.cc
File src/kudu/util/cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache-test.cc@331
PS8, Line 331: const int size_per_elem = cache_size() / kNumElems;
> As written I _think_ this test could also pass if it used LRU eviction. You
Ah, indeed: it's a good idea. Done.
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache-test.cc@355
PS8, Line 355: // to accomodate the elements from the second chunk.
> accommodate
Done
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache.h@222
PS8, Line 222:
> Nit: not necessary; it's obvious below.
Done
http://gerrit.cloudera.org:8080/#/c/12777/8/src/kudu/util/cache.h@228
PS8, Line 228:
:
:
> Should note that this type of cache doesn't support the NVM memory type.
Done
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 20 Mar 2019 06:00:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong, Adar Dembo, Hao Hao,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12777
to look at the new patch set (#5).
Change subject: [util] added cache with FIFO eviction policy
......................................................................
[util] added cache with FIFO eviction policy
Updated cache implementation in util/cache.{h,cc} to accommodate
the notion of a cache with FIFO eviction policy. Such cache is
useful for backing a TTL cache, i.e. a cache where an entry's life
cycle depends purely on the time when the entry was first inserted
into the cache.
Corresponding unit tests are updated as well.
Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
3 files changed, 441 insertions(+), 196 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/77/12777/5
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] added cache with FIFO eviction policy
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12777 )
Change subject: [util] added cache with FIFO eviction policy
......................................................................
Patch Set 9: Verified+1
unrelated flake in RemoteKsckTest.TestClusterWithLocation
--
To view, visit http://gerrit.cloudera.org:8080/12777
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If06fcf6c9860d0f384816414205f8791cbe53480
Gerrit-Change-Number: 12777
Gerrit-PatchSet: 9
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Hao Hao <ha...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 20 Mar 2019 16:05:35 +0000
Gerrit-HasComments: No