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/21 18:51:47 UTC
[kudu-CR] WIP [util] introduce TTL cache
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12825
Change subject: WIP [util] introduce TTL cache
......................................................................
WIP [util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries.
WIP:
* introduce and update TTL-related metric gauges
* maybe, update the behavior of the cache
to purge expired items upon lookup
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.cc
A src/kudu/util/ttl_cache_metrics.h
7 files changed, 568 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/1
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] introduce TTL cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 6:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@47
PS6, Line 47: The eviction
: // of entries continues until there is enough space to accommodate a new entry
: // or no entries are left in the cache
What happens if you add an entry that single-handledly exceeds the cache capacity? Worth a unit test?
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@76
PS6, Line 76: ptr_ = nullptr;
Is this necessary? Won't this initialization already have happened via the default constructor?
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@86
PS6, Line 86: explicit operator bool() const noexcept {
What does 'explicit' mean in this context? There are no arguments so what effect does this have?
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@90
PS6, Line 90: bool operator!() const noexcept {
I don't understand these operators well enough; are they allowed as per the Google Style Guide? See https://google.github.io/styleguide/cppguide.html#Implicit_Conversions in particular.
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@91
PS6, Line 91: return ptr_ == nullptr;
Seems like you could implement this in terms of bool() (or vice versa).
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@222
PS6, Line 222: if (PREDICT_TRUE(cache_->metrics_ != nullptr)) {
: if (entry_ptr->exp_time < MonoTime::Now()) {
: cache_->metrics_->evictions_expired->Increment();
: }
: }
Combine into one if statement?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 20:21:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 10: Code-Review+2
Carrying over Adar's +2 from PS8.
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 29 Mar 2019 02:12:14 +0000
Gerrit-HasComments: No
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 10:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache_test_metrics.cc
File src/kudu/util/ttl_cache_test_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache_test_metrics.cc@54
PS7, Line 54: tricUnit::kBytes,
> Ah, that's just test piece. Maybe in one of follow-up commits?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 29 Mar 2019 01:38:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: existing entry in the cache, if any.
It looks like if there is no expired entries, the cache entry is evicted by FIFO policy? If so, can you please comment here to make the eviction rule more clear?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 17:45:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 9:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12825/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12825/7//COMMIT_MSG@10
PS7, Line 10:
> nit: extra space.
Ah, that's intentional. I found this two-spaces-after-full-stop to be easier to read, and since we don't have strict style guide for descriptions of git changelists, I'm taking advantage of those loose rules :)
However, it seems we have some stricter rules for comments in the code, where there should be no duplicate spaces, even after full stop.
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache_test_metrics.cc
File src/kudu/util/ttl_cache_test_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache_test_metrics.cc@54
PS7, Line 54: tricUnit::kBytes,
> nit: rename it to test_ttl_cache_memory_usage?
Ah, that's just test piece. Maybe in one of follow-up commits?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
Gerrit-Comment-Date: Fri, 29 Mar 2019 00:25:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 8: Code-Review+2
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc
File src/kudu/util/ttl_cache_test_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@37
PS8, Line 37: their
Nit: you can drop 'their', it reads well either way but is more terse this way.
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@42
PS8, Line 42: found
find
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@46
PS8, Line 46: cached
Nit: a cached
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@51
PS8, Line 51: by
Nit: at
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 23:19:18 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: existing entry in the cache, if any.
> I don't quite understand what you mean here. The TTL cache has a static se
I meant '... how is it possible to get into a situation when an expired entry is still in the cache, but a non-expired one has been evicted already?'
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 18:46:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 9:
(4 comments)
> Patch Set 8: Code-Review+2
>
> (4 comments)
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc
File src/kudu/util/ttl_cache_test_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@37
PS8, Line 37: "
> Nit: you can drop 'their', it reads well either way but is more terse this
Done
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@42
PS8, Line 42: find
> find
Done
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@46
PS8, Line 46: a cach
> Nit: a cached
Done
http://gerrit.cloudera.org:8080/#/c/12825/8/src/kudu/util/ttl_cache_test_metrics.cc@51
PS8, Line 51: at
> Nit: at
Done
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
Gerrit-Comment-Date: Thu, 28 Mar 2019 23:32:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
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/12825
to look at the new patch set (#10).
Change subject: [util] introduce TTL cache
......................................................................
[util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy. A unit test to cover
the new functionality is added as well.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries. A follow-up patch will introduce automatic
scrubbing of the TTL cache, purging of expired entries periodically.
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.h
A src/kudu/util/ttl_cache_test_metrics.cc
A src/kudu/util/ttl_cache_test_metrics.h
8 files changed, 776 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/10
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] introduce TTL cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: s method returns corresponding handl
> Well, this comment was meant to refer to an entry with the same key that's
Hmm, that means if entries that haven't expired can be evicted while the one expired may not? If so, do you consider it as an issue? I saw you put a TODO 'add an option to evict expired entries on a periodic timer'. Are you consider that a must-do for using the TTL cache for master authz?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 18:35:07 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
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/12825
to look at the new patch set (#9).
Change subject: [util] introduce TTL cache
......................................................................
[util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy. A unit test to cover
the new functionality is added as well.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries. A follow-up patch will introduce automatic
scrubbing of the TTL cache, purging of expired entries periodically.
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.h
A src/kudu/util/ttl_cache_test_metrics.cc
A src/kudu/util/ttl_cache_test_metrics.h
8 files changed, 776 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/9
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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] WIP [util] introduce TTL cache
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/12825
to look at the new patch set (#4).
Change subject: WIP [util] introduce TTL cache
......................................................................
WIP [util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries.
WIP:
* introduce and update TTL-related metric gauges
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/tools/kudu-tool-test.cc
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.cc
A src/kudu/util/ttl_cache_metrics.h
7 files changed, 512 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/4
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
[kudu-CR] WIP [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 3:
(29 comments)
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG@13
PS3, Line 13: lazily purged
> You mentioned this offline, that this was fine because we expect the user o
I think that makes sense, but I think it's better to do that in a separate changelist.
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG@12
PS3, Line 12: Expired entries are not returned from the cache, but kept around until
: they are lazily purged upon cache reaching its capacity while
: accommodating more entries.
> Curious if you want to introduce a singleton thread that wakes up periodica
Yep, I'm planning to do that in a separate changelist.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@55
PS3, Line 55: using TTLTestCache = TTLCache<K, TestValue>;
> Seems like you could hardcode string as the key too, given how you're testi
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@62
PS3, Line 62: FLAGS_cache_force_single_shard = true;
: FLAGS_cache_memtracker_approximation_ratio = 0;
> Doc why you set these.
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@68
PS3, Line 68: auto h = cache.Get("key0");
: ASSERT_EQ(nullptr, h.get());
> Could combine negative lookups onto one line.
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@118
PS3, Line 118: ASSERT_NE(nullptr, put_h.get());
: ASSERT_NE(nullptr, put_h->ptr());
: ASSERT_EQ(100, put_h->ptr()->value);
> This seems to be a common pattern; maybe encapsulate in a helper method?
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@155
PS3, Line 155: auto h = cache.Get(key);
: ASSERT_NE(nullptr, h.get());
: ASSERT_NE(nullptr, h->ptr());
: ASSERT_EQ(i, h->ptr()->value);
> Any concern here (or on L170) that the test will run sufficiently slow that
Yes, I had that concern, but after running it multiple with 128 threads on ve0518 I didn't see any flakes. If any appears, we can change the cache's TTL.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@201
PS3, Line 201: ASSERT_NE(nullptr, h.get());
> Unnecessarily duplicated
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@40
PS3, Line 40: // A TTL cache. It's a cache of limited capacity with FIFO eviction policy.
: // All cache's entries have the same TTL. The TTL for cache's entries is
: // specified at the creation time of the cache and does not change during its
: // lifetime.
> Maybe reword as: "Cache with TTL-based time eviction and FIFO-based capacit
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@59
PS3, Line 59: V* ptr() const {
: return ptr_;
: }
> Why offer this at all? Is mutating a cached key's value an important use ca
As of now we don't need this, nope. Removed.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@91
PS3, Line 91: // TODO(aserbin): maybe, prohibit inheriting from this class instead?
> I don't think we need to be super protective on these matters; I think it's
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@93
PS3, Line 93:
: // Retrieve an entry from the cache and return it wrapped into a ref-counting
: // handle. If a non-expired entry exists for the specified key, this method
: // returns shared pointer for non-null handle. If there is no entry for the
: // specified key or the entry has expired at the time of the query,
: // nullptr wrapped into a ref-counted handle is returned.
> This (and Put()) is a little too much detail. The general "retrieves...if a
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@99
PS3, Line 99: std::shared_ptr<EntryHandle> Get(const K& key) {
> Why do Get()/Put() wrap EntryHandle in shared_ptr? The TTLCache itself does
I decided to use shared_ptr here because of the requirements: 1) handles should support reference counting 2) once handle is returned, it's easy to share it, if necessary, between multiple actors.
Yes, multiple Get() calls for the same key return "unrelated" shared pointers. I don't think it's an issue.
What would you return from here if not shared_ptr if not building some home-grown wrapper? Returning unique_ptr would make it harder to use: e.g., I don't think using Cache itself with its UniqueHandle is handy. However, if we want this TTL cache to follow the suite, maybe that's an option. What do you think?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@114
PS3, Line 114: // TODO(aserbin) use make_shared?
> Yes, we should do this to reduce the number of allocations.
Ack
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@115
PS3, Line 115: return std::shared_ptr<EntryHandle>(
> Something here seems off. If I Get() the same key twice, and let both Entry
I don't think something is off here. Every handle returned by Get() aggregates distinct instance of Cache::UniqueHandle, and Cache itself is declared to be safe with regard to concurrent queries.
I added the test scenario you described.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@115
PS3, Line 115: return std::shared_ptr<EntryHandle>(
: new EntryHandle(DCHECK_NOTNULL(entry_ptr->val_ptr), std::move(h)));
> Just to make sure I understand the ownership model of this; are the shared_
No, they are not counting "together. See the response for Adar's comment at line 99
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@123
PS3, Line 123: The 'charge' parameter specifies
: // the charge to associate with the entry with regard to the overall
: // cache's capacity.
> What's the use case for this vs. letting the charge get calculated automati
As far as I can see, automatic calculation of memory consumption is pretty much useless unless some blob of raw memory stored in the cache. The use case for supplying the charge for an item explicitly comes from the usage of this TTL cache in sentry_authz_provider.{cc,h}
I thought that requiring some particular interface for the values stored in the cache (e.g., 'ssize_t memory_footprint() const') might be a better choice, but I seems to be ugly as well, so I abandoned it.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@126
PS3, Line 126: std::shared_ptr<EntryHandle> Put(const K& key,
> Curious why Put() returns an EntryHandle instead of void? What's the use ca
The use case is in sentry_authz_provider.cc. The idea is to use the same pattern of using cache's handles while retrieving and storing entries in the cache.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@129
PS3, Line 129: // TODO(aserbin): add footprint for structures used by Cache's
: // internal housekeeping (RL handles, etc.)
> This is KUDU-1091, right? Does it need to be tracked in TTLCache as well as
Ah, indeed -- thank you for the reference.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@131
PS3, Line 131: Cache::PendingHandle* pending = cache_->Allocate(key, sizeof(Entry), charge);
> Could you store this in a unique_ptr that uses Cache::Free as a deleter? Th
That would not be move(), that would be release(). I think it's better to change the signature of Cache::Allocate() to consume unique_ptr instead of raw pointer and update all corresponding call sites. I think that will be better put in a separate changelist. What do you think?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@132
PS3, Line 132: if (!pending) {
: LOG(WARNING) << strings::Substitute(
: "failed to allocate an entry with charge of $0", charge);
: return nullptr;
: }
> Maybe just CHECK on this instead?
For some reason, I thought Cache::Allocate() would return nullptr if it cannot allocate space to fit the cache given its capacity, but it seems current Cache::Allocate() doesn't handle that at all. In that case CHECK() here is better, of course.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@138
PS3, Line 138: memcpy(cache_->MutableValue(pending), &entry, sizeof(Entry));
> Right now, we:
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@139
PS3, Line 139: Cache::UniqueHandle h(cache_->Insert(pending, eviction_cb_.get()),
> This also evicts an existing entry with key 'key', right? Maybe doc that?
That is documented in the method's comment; I also added a comment for the Insert() at-the-site.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@151
PS3, Line 151: // Pointer to the 'value'.
> Should probably say something about ownership here. Would it even be possib
Since that's all about storing object in some opaque memory blobs in underlying Cache, it doesn't make much sense to use unique_ptr instead of raw pointer here: regular rules of variables/member scoping do not work.
I added corresponding comment regarding the ownership of the memory for 'val_ptr'.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@157
PS3, Line 157: // The eviction callback that the underlying FIFO cache uses to notify
: // about reaching zero reference count for its entries.
> Reword as "Callback invoked by the underlying FIFO cache when a cached entr
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@166
PS3, Line 166: auto* entry_ptr = reinterpret_cast<Entry*>(val.mutable_data());
: DCHECK(entry_ptr);
: delete entry_ptr->val_ptr;
> Nit: rather than the explicit delete, maybe cast and store into a unique_pt
That sounds too fancy to me :) Do you mind if I leave it as is? I think this way it's a bit more readable.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc
File src/kudu/util/ttl_cache_metrics.cc:
PS3:
> Seems inappropriate for these metrics to be defined for the "TTL cache", be
Right -- I had some back-and-forth where to put this, but eventually dumped it in ttl_cache_metircs.cc. I think I'll move and split that once the part with TTL-related gauges settles in.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc@41
PS3, Line 41: cache_misses
> ttl_cache_misses
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc@49
PS3, Line 49: cache_hits
> ttl_cache_hits
Done
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 3
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, 27 Mar 2019 06:53:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 7:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@47
PS6, Line 47: ach: the earliest
: // added entry is evicted first. Since the TTL setting is static and applies
: // to every entry in the cache, the FI
> What happens if you add an entry that single-handledly exceeds the cache ca
As I understand this question that is not specific to the TTLCache, but it's a question about the behavior of the underlying Cache.
I already though about this, and as far as I can see, current Cache's implementation simply adds such an entry. I thought that Cache::Allocate() would be the first piece that would report a problem if size of an entry exceeds capacity for the cache, returning null handle. But that's not the case, as far as I can see. With current implementation of Cache, upon inserting an entry that exceeds cache's capacity all other entries will be evicted from the cache and the new giant will be added. Nothing special at this point.
If my analysis is correct, do you think we should fix current Cache's behavior in such a scenario?
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@76
PS6, Line 76: handle_ = std::move(other.handle_);
> Is this necessary? Won't this initialization already have happened via the
Maybe I'm missing something here, but what if the instance was created with non-default constructor? In that case I think it's better to clear the ptr_.
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@86
PS6, Line 86: // Copying of entry handles is explicitly prohibited.
> What does 'explicit' mean in this context? There are no arguments so what e
That means the operator will apply only for explicit conversions. static_cast<> and if (...) are among those: https://en.cppreference.com/w/cpp/language/cast_operator
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@90
PS6, Line 90: explicit operator bool() const noexcept {
> I don't understand these operators well enough; are they allowed as per the
Ah, I think this is no longer needed once we have that explicit type cast operator.
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@91
PS6, Line 91: return ptr_ != nullptr;
> Seems like you could implement this in terms of bool() (or vice versa).
Ack
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@222
PS6, Line 222: if (PREDICT_TRUE(cache_->metrics_ != nullptr) &&
: entry_ptr->exp_time < MonoTime::Now()) {
: cache_->metrics_->evictions_expired->Increment();
: }
: d
> Combine into one if statement?
Done
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: s method returns corresponding handl
> Ah sorry, my bad, somehow I thought entry_ttl_ is a per entry value.
Np, I just wanted to make sure I didn't miss anything. If that was exactly the issue you thought about and my explanation made it clear that we don't have such a problem with our caching approach, that's great :)
I also update the class-wide comment on the TTLCache, trying to make it clear wrt to the issue you described.
I don't anticipate the need to have per-entry TTL setting. If there is a need to update the TTL setting, it's necessary to reset the cache. We could allow to dynamically update the TTL if it's increasing, but I'm not sure it's worth it.
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
Gerrit-Comment-Date: Thu, 28 Mar 2019 21:59:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 4:
Also is this still WIP? Do you intend on adding the TTL-metric gauges in this patch? Or a followup?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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, 27 Mar 2019 19:48:05 +0000
Gerrit-HasComments: No
[kudu-CR] [util] introduce TTL cache
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/12825
to look at the new patch set (#5).
Change subject: [util] introduce TTL cache
......................................................................
[util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy. A unit test to cover
the new functionality is added as well.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries. A follow-up patch will introduce
automatic purging of expired entries on a periodic timer.
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.h
A src/kudu/util/ttl_cache_test_metrics.cc
A src/kudu/util/ttl_cache_test_metrics.h
8 files changed, 769 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/5
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 5
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] WIP [util] introduce TTL cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
LGTM, though Adar's question about shared_ptrs/unique_ptrs is probably worth considering.
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@75
PS4, Line 75: {}
Is there a difference between this and nullptr?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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, 27 Mar 2019 19:47:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache-test.cc@35
PS2, Line 35: using std::shared_ptr;
> warning: using decl 'shared_ptr' is unused [misc-unused-using-decls]
Done
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@41
PS2, Line 41: entiries
> nit: entries
Done
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@42
PS2, Line 42: do
> nit: does
Done
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@63
PS2, Line 63: V& value() const {
: DCHECK(ptr_);
: return *ptr_;
: }
> Is this used anywhere right now?
Yes, after a few iterations on authz provider that's now used in sentry_authz_provider.
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@109
PS2, Line 109: // If the item has expired already, return null handle. The underlying
: // cache will purge the expired entry when necessary upon accommodating
: // new entries being added into the cache, if any.
> Curious what the line of thought behind this is -- it seems innocuous enoug
The way how the TTL cache is used assumes the client will fetch and put a new entry once it's detected an entry is expired. At least for the current use case in Sentry authz provider that's the case.
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@128
PS2, Line 128: size_t charge
> Perhaps it's worth having a default of kAutomaticCharge?
Done
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 2
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: Mon, 25 Mar 2019 19:45:12 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 7: Code-Review+2
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache.h@46
PS7, Line 46: with FIFO eviction policy
Nit: you can drop this part; the rest of the sentence mentions that eviction is done using FIFO.
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache.h@50
PS7, Line 50: evicting
Nit: "to evicting"
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@47
PS6, Line 47: ach: the earliest
: // added entry is evicted first. Since the TTL setting is static and applies
: // to every entry in the cache, the FI
> As I understand this question that is not specific to the TTLCache, but it'
Probably not worth it, though we may want to think about how to protect ourselves in the event that Sentry sends us a massive response.
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@76
PS6, Line 76: handle_ = std::move(other.handle_);
> Maybe I'm missing something here, but what if the instance was created with
Ah, right, you want to propagate a nullptr to other.ptr_. Maybe this would be clearer?
ptr_ = other.ptr_;
other.ptr_ = nullptr;
Anyway it's not a big deal; the current code is fine too.
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
Gerrit-Comment-Date: Thu, 28 Mar 2019 22:15:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 9: Code-Review+1
(3 comments)
LGTM, just some minor nits.
http://gerrit.cloudera.org:8080/#/c/12825/7//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12825/7//COMMIT_MSG@10
PS7, Line 10:
nit: extra space.
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: s method returns corresponding handl
> Np, I just wanted to make sure I didn't miss anything. If that was exactly
LGTM.
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache_test_metrics.cc
File src/kudu/util/ttl_cache_test_metrics.cc:
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache_test_metrics.cc@54
PS7, Line 54: tricUnit::kBytes,
nit: rename it to test_ttl_cache_memory_usage?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
Gerrit-Comment-Date: Thu, 28 Mar 2019 23:52:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: existing entry in the cache, if any.
> Hmm, that means if entries that haven't expired can be evicted while the on
I don't quite understand what you mean here. The TTL cache has a static setting for the TTL of its entries, so _all_ added entries have the same TTL. The entries added first always have earliest expiration time. With that, how is it possible to get into a situation when an expired entry is still in the cache, but a non-expired one has been removed already?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 18:44:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 4:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/cache.cc@592
PS4, Line 592: // TODO(aserbin): account for the footprint of structures used by Cache's
Could you replace 'aserbin' with KUDU-1091?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@99
PS3, Line 99: // out of scope.
> I decided to use shared_ptr here because of the requirements: 1) handles sh
Unless there's a specific use case for EntryHandle sharing, I'd suggest wrapping with unique_ptr. Otherwise there's just unnecessary complexity. Better yet, could we return raw EntryHandles and avoid allocating EntryHandles on the heap at all?
This has got me thinking about FileCache, which also wraps its handles with shared_ptr but then takes the extra step of facilitating handle sharing via an extra map (and lock) on top of the underlying ShardedLRUCache. The original design called for handle sharing to support multiple openers of the same file, but I don't think that actually happens anywhere in Kudu, and the implementation could be greatly simplified by removing that wrapping (and the sharing).
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@115
PS3, Line 115: // TODO(aserbin) use make_shared?
> I don't think something is off here. Every handle returned by Get() aggreg
Sorry, I forgot that although Cache::UniqueHandle is a unique_ptr, the cached key/value pair is still reference counted.
This is another reason to try and simplify ownership as much as possible; with all the different kinds of handles at play, it can be easy to miss the ownership semantics of each link along the chain:
EntryHandle (currently shared) --> Cache::UniqueHandle (exclusive) --> RLHandle (shared)
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@131
PS3, Line 131: Entry* entry = reinterpret_cast<Entry*>(cache_->MutableValue(pending));
> That would not be move(), that would be release(). I think it's better to
That's fine with me.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@166
PS3, Line 166: delete entry_ptr->val_ptr;
: }
:
> That sounds too fancy to me :) Do you mind if I leave it as is? I think t
That's fine, just leave it as-is.
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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, 27 Mar 2019 17:28:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
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/12825
to look at the new patch set (#8).
Change subject: [util] introduce TTL cache
......................................................................
[util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy. A unit test to cover
the new functionality is added as well.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries. A follow-up patch will introduce automatic
scrubbing of the TTL cache, purging of expired entries periodically.
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.h
A src/kudu/util/ttl_cache_test_metrics.cc
A src/kudu/util/ttl_cache_test_metrics.h
8 files changed, 776 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/8
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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] introduce TTL cache
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/12825
to look at the new patch set (#7).
Change subject: [util] introduce TTL cache
......................................................................
[util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy. A unit test to cover
the new functionality is added as well.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries. A follow-up patch will introduce automatic
scrubbing of the TTL cache, purging of expired entries periodically.
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.h
A src/kudu/util/ttl_cache_test_metrics.cc
A src/kudu/util/ttl_cache_test_metrics.h
8 files changed, 778 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/7
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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] WIP [util] introduce TTL cache
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 3:
(27 comments)
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG@12
PS3, Line 12: Expired entries are not returned from the cache, but kept around until
: they are lazily purged upon cache reaching its capacity while
: accommodating more entries.
Curious if you want to introduce a singleton thread that wakes up periodically to purge expired entries. Otherwise the cache capacity is never reclaimed, even if the only workload to use it is long gone.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@55
PS3, Line 55: using TTLTestCache = TTLCache<K, TestValue>;
Seems like you could hardcode string as the key too, given how you're testing.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@62
PS3, Line 62: FLAGS_cache_force_single_shard = true;
: FLAGS_cache_memtracker_approximation_ratio = 0;
Doc why you set these.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@68
PS3, Line 68: auto h = cache.Get("key0");
: ASSERT_EQ(nullptr, h.get());
Could combine negative lookups onto one line.
Below too.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@118
PS3, Line 118: ASSERT_NE(nullptr, put_h.get());
: ASSERT_NE(nullptr, put_h->ptr());
: ASSERT_EQ(100, put_h->ptr()->value);
This seems to be a common pattern; maybe encapsulate in a helper method?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@155
PS3, Line 155: auto h = cache.Get(key);
: ASSERT_NE(nullptr, h.get());
: ASSERT_NE(nullptr, h->ptr());
: ASSERT_EQ(i, h->ptr()->value);
Any concern here (or on L170) that the test will run sufficiently slow that the entries may be evicted by the time we call Get()? Could you loop in TSAN mode with stress threads?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache-test.cc@201
PS3, Line 201: ASSERT_NE(nullptr, h.get());
Unnecessarily duplicated
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@40
PS3, Line 40: // A TTL cache. It's a cache of limited capacity with FIFO eviction policy.
: // All cache's entries have the same TTL. The TTL for cache's entries is
: // specified at the creation time of the cache and does not change during its
: // lifetime.
Maybe reword as: "Cache with TTL-based time eviction and FIFO-based capacity eviction. All entries have the same static TTL specified at cache creation time."
Should also mention here (or maybe in Get()/Put()) how returned handles affect eviction (either time or capacity based) as well as destruction of the underlying cached value. I think the answers are "cached key/value pairs may still be evicted even with an outstanding handle, but a cached value won't be destroyed until a returned value goes out of scope".
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@59
PS3, Line 59: V* ptr() const {
: return ptr_;
: }
Why offer this at all? Is mutating a cached key's value an important use case?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@91
PS3, Line 91: // TODO(aserbin): maybe, prohibit inheriting from this class instead?
I don't think we need to be super protective on these matters; I think it's safe to assume that nobody is going to extend this class, so a virtual destructor is unnecessary.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@93
PS3, Line 93:
: // Retrieve an entry from the cache and return it wrapped into a ref-counting
: // handle. If a non-expired entry exists for the specified key, this method
: // returns shared pointer for non-null handle. If there is no entry for the
: // specified key or the entry has expired at the time of the query,
: // nullptr wrapped into a ref-counted handle is returned.
This (and Put()) is a little too much detail. The general "retrieves...if an entry exists...if no entry exists" structure is good, but I think you can elide some of the associated detail, especially since the implementations are inline and can be inspected easily.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@99
PS3, Line 99: std::shared_ptr<EntryHandle> Get(const K& key) {
Why do Get()/Put() wrap EntryHandle in shared_ptr? The TTLCache itself doesn't actually enable sharing of EntryHandles; it looks like it always returns a newly allocated one.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@114
PS3, Line 114: // TODO(aserbin) use make_shared?
Yes, we should do this to reduce the number of allocations.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@115
PS3, Line 115: return std::shared_ptr<EntryHandle>(
Something here seems off. If I Get() the same key twice, and let both EntryHandles go out of scope, won't I end up double freeing the cached value? Can you add a test for this?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@123
PS3, Line 123: The 'charge' parameter specifies
: // the charge to associate with the entry with regard to the overall
: // cache's capacity.
What's the use case for this vs. letting the charge get calculated automatically based on memory consumption?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@126
PS3, Line 126: std::shared_ptr<EntryHandle> Put(const K& key,
Curious why Put() returns an EntryHandle instead of void? What's the use case you're building for (and maybe doc it)?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@129
PS3, Line 129: // TODO(aserbin): add footprint for structures used by Cache's
: // internal housekeeping (RL handles, etc.)
This is KUDU-1091, right? Does it need to be tracked in TTLCache as well as in Cache?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@131
PS3, Line 131: Cache::PendingHandle* pending = cache_->Allocate(key, sizeof(Entry), charge);
Could you store this in a unique_ptr that uses Cache::Free as a deleter? Then move it into the Insert() call on L139?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@132
PS3, Line 132: if (!pending) {
: LOG(WARNING) << strings::Substitute(
: "failed to allocate an entry with charge of $0", charge);
: return nullptr;
: }
Maybe just CHECK on this instead?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@138
PS3, Line 138: memcpy(cache_->MutableValue(pending), &entry, sizeof(Entry));
Right now, we:
1. Allocate space for an Entry in Allocate().
2. Declare another Entry on the stack and populate it.
3. Copy the stack-allocated Entry into the cache-allocated Entry.
Instead, could we cast the result of MutableValue() into an Entry*, then populate it directly? Should let us avoid the copy in step 3.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@139
PS3, Line 139: Cache::UniqueHandle h(cache_->Insert(pending, eviction_cb_.get()),
This also evicts an existing entry with key 'key', right? Maybe doc that?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@151
PS3, Line 151: // Pointer to the 'value'.
Should probably say something about ownership here. Would it even be possible to convert val_ptr into a unique_ptr? Or does that not work?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@157
PS3, Line 157: // The eviction callback that the underlying FIFO cache uses to notify
: // about reaching zero reference count for its entries.
Reword as "Callback invoked by the underlying FIFO cache when a cached entry's reference count reaches zero and is evicted."
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@166
PS3, Line 166: auto* entry_ptr = reinterpret_cast<Entry*>(val.mutable_data());
: DCHECK(entry_ptr);
: delete entry_ptr->val_ptr;
Nit: rather than the explicit delete, maybe cast and store into a unique_ptr, then just let it go out of scope?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc
File src/kudu/util/ttl_cache_metrics.cc:
PS3:
Seems inappropriate for these metrics to be defined for the "TTL cache", because that's generic enough that you could imagine it being used for a number of different purposes. Like, wouldn't these metrics be better suited for the Sentry privilege cache? Then if someone used TTLCache for yet another cache, we could differentiate between the two.
You could make a similar argument for FileCache but I think it's weaker, because the point of the FileCache is to limit the number of open fds, and to do that effectively you'd need one instance for all file open/close.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc@41
PS3, Line 41: cache_misses
ttl_cache_misses
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache_metrics.cc@49
PS3, Line 49: cache_hits
ttl_cache_hits
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 3
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: Mon, 25 Mar 2019 22:56:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
[util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy. A unit test to cover
the new functionality is added as well.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries. A follow-up patch will introduce automatic
scrubbing of the TTL cache, purging of expired entries periodically.
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Reviewed-on: http://gerrit.cloudera.org:8080/12825
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.h
A src/kudu/util/ttl_cache_test_metrics.cc
A src/kudu/util/ttl_cache_test_metrics.h
8 files changed, 776 insertions(+), 1 deletion(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 11
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] introduce TTL cache
Posted by "Hao Hao (Code Review)" <ge...@cloudera.org>.
Hao Hao has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: s method returns corresponding handl
> I meant '... how is it possible to get into a situation when an expired ent
Ah sorry, my bad, somehow I thought entry_ttl_ is a per entry value.
Btw, do you see any needs to have different TTL for each entry receptively? What happens if the user wants to update entry_ttl_ after using the cache for sometime.
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 20:25:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 3:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12825/3//COMMIT_MSG@13
PS3, Line 13: lazily purged
You mentioned this offline, that this was fine because we expect the user of the cache to Put something into the cache upon learning that the key doesn't exist, which will purge the existing values. IMO as a utility class, it'd be simpler if the cache purged automatically. That said, if you can make a case for this approach being better, mind adding that here?
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@115
PS3, Line 115: return std::shared_ptr<EntryHandle>(
: new EntryHandle(DCHECK_NOTNULL(entry_ptr->val_ptr), std::move(h)));
Just to make sure I understand the ownership model of this; are the shared_ptrs returned by Get() counting "together"? E.g. if it's called twice so we have two shared_ptrs to the same key, and one of the shared_ptrs is destroyed, will the other be in tact? Since we're creating a new object on the heap, I wouldn't think so, but I'm not sure I have a clear mental model of shared_ptr. If they aren't counted together, what's the point of returning a shared_ptr in the first place?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 3
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, 26 Mar 2019 18:01:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 7:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache.h@46
PS7, Line 46: with FIFO eviction policy
> Nit: you can drop this part; the rest of the sentence mentions that evictio
Done
http://gerrit.cloudera.org:8080/#/c/12825/7/src/kudu/util/ttl_cache.h@50
PS7, Line 50: evicting
> Nit: "to evicting"
Done
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@47
PS6, Line 47: ach: the earliest
: // added entry is evicted first. Since the TTL setting is static and applies
: // to every entry in the cache, the FI
> Probably not worth it, though we may want to think about how to protect our
All right, it might be an option, I think -- whether to reject such huge entries or accept them anyways as it is now. I think it's a good idea to opt in TTLCache for the new behavior, while by default keep everything else with the legacy behavior.
I think I'll address that in a separate changelist.
http://gerrit.cloudera.org:8080/#/c/12825/6/src/kudu/util/ttl_cache.h@76
PS6, Line 76: handle_ = std::move(other.handle_);
> Ah, right, you want to propagate a nullptr to other.ptr_. Maybe this would
Done
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
Gerrit-Comment-Date: Thu, 28 Mar 2019 23:09:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: WIP [util] introduce TTL cache
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/12825/2//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12825/2//COMMIT_MSG@18
PS2, Line 18: * maybe, update the behavior of the cache
: to purge expired items upon lookup
+1, but would be curious if you've got an argument for not doing this.
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@41
PS2, Line 41: entiries
nit: entries
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@42
PS2, Line 42: do
nit: does
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@63
PS2, Line 63: V& value() const {
: DCHECK(ptr_);
: return *ptr_;
: }
Is this used anywhere right now?
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@109
PS2, Line 109: // If the item has expired already, return null handle. The underlying
: // cache will purge the expired entry when necessary upon accommodating
: // new entries being added into the cache, if any.
Curious what the line of thought behind this is -- it seems innocuous enough to purge now? I guess it saves on some removal calls up to the point where we're full, but doing so consumes more memory for those expired entries (albeit bounded by our configured limit).
http://gerrit.cloudera.org:8080/#/c/12825/2/src/kudu/util/ttl_cache.h@128
PS2, Line 128: size_t charge
Perhaps it's worth having a default of kAutomaticCharge?
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 2
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: Sat, 23 Mar 2019 00:06:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] WIP [util] introduce TTL cache
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/12825
to look at the new patch set (#2).
Change subject: WIP [util] introduce TTL cache
......................................................................
WIP [util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries.
WIP:
* introduce and update TTL-related metric gauges
* maybe, update the behavior of the cache
to purge expired items upon lookup
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.cc
A src/kudu/util/ttl_cache_metrics.h
6 files changed, 509 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/2
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
Gerrit-PatchSet: 2
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] introduce TTL cache
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/12825
to look at the new patch set (#6).
Change subject: [util] introduce TTL cache
......................................................................
[util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy. A unit test to cover
the new functionality is added as well.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries. A follow-up patch will introduce automatic
scrubbing of the TTL cache, purging of expired entries periodically.
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.h
A src/kudu/util/ttl_cache_test_metrics.cc
A src/kudu/util/ttl_cache_test_metrics.h
8 files changed, 779 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/6
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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)
[kudu-CR] WIP [util] introduce TTL cache
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/12825
to look at the new patch set (#3).
Change subject: WIP [util] introduce TTL cache
......................................................................
WIP [util] introduce TTL cache
This changelist introduces TTL cache based on the cache implementation
from util/cache.{h,cc} with FIFO eviction policy.
Expired entries are not returned from the cache, but kept around until
they are lazily purged upon cache reaching its capacity while
accommodating more entries.
WIP:
* introduce and update TTL-related metric gauges
Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
---
M src/kudu/util/CMakeLists.txt
M src/kudu/util/cache.cc
A src/kudu/util/ttl_cache-test.cc
A src/kudu/util/ttl_cache.h
A src/kudu/util/ttl_cache_metrics.cc
A src/kudu/util/ttl_cache_metrics.h
6 files changed, 508 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/25/12825/3
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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] introduce TTL cache
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12825 )
Change subject: [util] introduce TTL cache
......................................................................
Patch Set 4:
(7 comments)
> Also is this still WIP? Do you intend on adding the TTL-metric
> gauges in this patch? Or a followup?
I removed the WIP piece: TTLCache-specific metrics gauges are there.
I'm planning to add a follow-up patch adding the 'scrubbing' functionality: remove expired entries from the cache periodically.
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/cache.cc@592
PS4, Line 592: // TODO(aserbin): account for the footprint of structures used by Cache's
> Could you replace 'aserbin' with KUDU-1091?
Whoops, it meant to be KUDU-1091, sure.
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@75
PS4, Line 75: {}
> Is there a difference between this and nullptr?
At least, this one has less symbols :)
This is not relevant anymore since the way of binding metrics to a TTL cache has changed.
http://gerrit.cloudera.org:8080/#/c/12825/4/src/kudu/util/ttl_cache.h@134
PS4, Line 134: existing entry in the cache, if any.
> It looks like if there is no expired entries, the cache entry is evicted by
Well, this comment was meant to refer to an entry with the same key that's being inserted.
In short, when this cache (TTLCache) is at capacity and needs to accommodate a new entry, it evicts already existing entries regardless of expiration using FIFO policy. That means that the earliest entry is evicted first. If after that it's still not enough free space in the cache, the eviction process continues doing the same again and again until it's enough space to accommodate a new entry or no entries left in the cache.
I clarified on both: a comment on the of FIFO eviction policy has been added into the class-wide comment for the TTLCache class.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@99
PS3, Line 99: // out of scope.
> Unless there's a specific use case for EntryHandle sharing, I'd suggest wra
Done
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@115
PS3, Line 115: // TODO(aserbin) use make_shared?
> Sorry, I forgot that although Cache::UniqueHandle is a unique_ptr, the cach
Yes, I totally agree that's confusing. Maybe, let's start with not sharing EntryHandle returned from here.
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@131
PS3, Line 131: Entry* entry = reinterpret_cast<Entry*>(cache_->MutableValue(pending));
> That's fine with me.
Ack
http://gerrit.cloudera.org:8080/#/c/12825/3/src/kudu/util/ttl_cache.h@166
PS3, Line 166: delete entry_ptr->val_ptr;
: }
:
> That's fine, just leave it as-is.
Ack
--
To view, visit http://gerrit.cloudera.org:8080/12825
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8aa0ebe6b26bf34ca0e02bebbdb6a94f6b00621
Gerrit-Change-Number: 12825
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: Thu, 28 Mar 2019 18:17:21 +0000
Gerrit-HasComments: Yes