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/04/30 19:37:42 UTC
[kudu-CR] [util] interface to invalidate cache entries
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13196
Change subject: [util] interface to invalidate cache entries
......................................................................
[util] interface to invalidate cache entries
Added a means to invalidate cache entries.
Added corresponding tests as well.
Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
6 files changed, 313 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/13196/1
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@266
PS2, Line 266: For example, in case of
: // multi-shard cache, when the 'iteration_func' returns 'false',
: // the invalidation at current shard stops and switches to the next
: // non-yet-processed shard, if any is present.
> Since it's a bit of a generic interface, I don't think that having just a b
Yeah but these examples don't reflect real use cases in Kudu yet, nor are such use cases planned. So why bother ensuring the interface can support them? We can always revisit this should they materialize, and in the meantime we'll benefit from the reduced complexity.
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 19:06:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc@63
PS2, Line 63: const Cache::ValidityFunc Cache::kInvalidateAllEntriesFunc = [](
: Slice /* key */, Slice /* value */) {
: return false;
: };
:
: const Cache::IterationFunc Cache::kIterateOverAllEntriesFunc = [](
: size_t /* valid_entries_num */, size_t /* valid_entries_num */) {
: return true;
: };
> These seem more expressive than they need to be for the purpose of invalida
Ah, looking at https://gerrit.cloudera.org/c/13201/5/src/kudu/util/ttl_cache.h, looks like the answer is yes.
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 19:31:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 03:26:27 +0000
Gerrit-HasComments: No
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
[util] interface to invalidate cache entries
Added a means to invalidate entries in Cache. Basically, it's a way
to iterate over all entries in a cache while having an ability to
remove an entry based on a set of criteria specified using an entry's
key and value. The generic usage pattern of the introduced invalidation
mechanism is to purge of entries matching criteria based on their keys
and values.
In particular, that's useful for periodic scrubbing tasks which get
rid of expired entries in a TTLCache. It's about invalidating expired
entries in a TTL cache even if the cache hasn't reached its capacity
yet. The latter, being an optimization, allows for more effective
usage of the OS memory in the presence of multiple concurrent processes.
Added corresponding tests as well.
Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Reviewed-on: http://gerrit.cloudera.org:8080/13196
Tested-by: Kudu Jenkins
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
6 files changed, 354 insertions(+), 4 deletions(-)
Approvals:
Kudu Jenkins: Verified
Adar Dembo: Looks good to me, but someone else must approve
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13196
to look at the new patch set (#2).
Change subject: [util] interface to invalidate cache entries
......................................................................
[util] interface to invalidate cache entries
Added a means to invalidate entries in Cache. Basically, it's a way
to iterate over all entries in a cache while having an ability to
remove an entry based on a set of criteria specified using an entry's
key and value. The generic usage pattern of the introduced invalidation
mechanism is to purge of entries matching criteria based on their keys
and values.
In particular, that's useful for periodic scrubbing tasks which get
rid of expired entries in a TTLCache. It's about invalidating expired
entries in a TTL cache even if the cache hasn't reached its capacity
yet. The latter, being an optimization, allows for more effective
usage of the OS memory in the presence of multiple concurrent processes.
Added corresponding tests as well.
Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
6 files changed, 354 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/13196/2
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 1:
(12 comments)
http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG@7
PS1, Line 7: [util] interface to invalidate cache entries
> What motivated this patch? The CLI tool outright reset the entire TTL cache
Ah, it seems I cut corners with commit messages while trying to put up a patch for review sooner :) I added more information into the commit message.
Thank you for the prompt review!
http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG@7
PS1, Line 7: [util] interface to invalidate cache entries
> Answering my own question: this supports the TTL cache scrubbing thread. Co
Done
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@246
PS1, Line 246: typedef std::function<bool(size_t /* valid_entries_num */,
> I think it'd be simpler to combine this into the ValidityFunc. Invalidate c
I seems to me that keeping ValidityFunc and IterationFunc orthogonal is a bit clearer. I tried to go with that combined-enum approach for return type of would-be-validity-functor, and I didn't like it.
I opted for having InvalidationControl stucture with default parameters for its constructor. I found it easier to use than a method with two parameters.
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@251
PS1, Line 251: a
> an
Done
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@252
PS1, Line 252: optional
> If it's optional, perhaps wrap it in boost::optional?
Ah, that's outdated. Originally, it was optional since it was a parameter with default value. However, code style doesn't allow parameters by default in signatures of virtual functions.
I updated the comment.
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@255
PS1, Line 255: when the provided functor is non-null
> Both functors are passed by cref so how could either be null?
My bad -- this comment is outdated.
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@256
PS1, Line 256: shard
> Sharding is an implementation detail and not a requirement for implementing
Ah, that was an explicit attempt to avoid possible backlash due to 'vagueness'.
All right, I think I can make it generic and use a sharded cache as an example.
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@260
PS1, Line 260: virtual size_t Invalidate(const ValidityFunc& validity_func,
> It's not clear from the commit message (or context) why all of this is usef
I added more information into the commit message and more in-line doc for the IterationFunc.
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@268
PS1, Line 268: examing
> examine
Done
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc@64
PS1, Line 64: Slice, Slice) {
> warning: all parameters should be named in a function [readability-named-pa
Done
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc@69
PS1, Line 69: size_t, size_t) {
> warning: all parameters should be named in a function [readability-named-pa
Done
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc@508
PS1, Line 508: std::lock_guard<MutexType> l(mutex_);
> If holding the mutex_ while calling the functors is a requirement, should d
I added a comment in the in-line doc for new methods in cache.h
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 07:20:37 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13196
to look at the new patch set (#3).
Change subject: [util] interface to invalidate cache entries
......................................................................
[util] interface to invalidate cache entries
Added a means to invalidate entries in Cache. Basically, it's a way
to iterate over all entries in a cache while having an ability to
remove an entry based on a set of criteria specified using an entry's
key and value. The generic usage pattern of the introduced invalidation
mechanism is to purge of entries matching criteria based on their keys
and values.
In particular, that's useful for periodic scrubbing tasks which get
rid of expired entries in a TTLCache. It's about invalidating expired
entries in a TTL cache even if the cache hasn't reached its capacity
yet. The latter, being an optimization, allows for more effective
usage of the OS memory in the presence of multiple concurrent processes.
Added corresponding tests as well.
Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
---
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
6 files changed, 354 insertions(+), 4 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/96/13196/3
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2: Verified+1
unrelated flakes in:
* HmsSentryConfigurations/AlterTableRandomized.TestRandomSequence/1
* KuduTest.TestWebUIDoesNotCrashCluster
* HmsSentryConfigurations/MasterStressTest.Test/0
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 17:40:19 +0000
Gerrit-HasComments: No
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@243
PS2, Line 243: from the oldest (less relevant) entries to the newest (more relevant) ones.
> Does the order depend on the kind of recency list it is?
Caches with both LRU and FIFO eviction policies keep their lists organized the way where the best candidates for eviction is the beginning of the list. Of course, that's an implementation detail, but this comment reflects the contract the implementation should follow to make invalidation more effective.
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@265
PS2, Line 265: Ivalidate
> Invalidate
Done
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@266
PS2, Line 266: For example, in case of
: // multi-shard cache, when the 'iteration_func' returns 'false',
: // the invalidation at current shard stops and switches to the next
: // non-yet-processed shard, if any is present.
> I agree, I wasn't sure what the motivation behind these functions were, giv
My concern was concurrent access to the cache from 'outside' while running the invalidation from 'inside', so I wanted to have a means to control the number of invalidated entries per one run of lazy scrubbing threads (like in the TTLCache) to reduce contention. As of now, I addressed that in the TTLCache and how it's used in the SentryPrivilegesFetcher. Basically, I don't want the scrubbing thread to behave like another JVM's GC-like global lock.
So, those use-cases have materialized in the follow-up changelists. If you feel strongly against it anyway, let's continue the discussion.
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@304
PS2, Line 304: to for
> nit: for
Done
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc@63
PS2, Line 63: const Cache::ValidityFunc Cache::kInvalidateAllEntriesFunc = [](
: Slice /* key */, Slice /* value */) {
: return false;
: };
:
: const Cache::IterationFunc Cache::kIterateOverAllEntriesFunc = [](
: size_t /* valid_entries_num */, size_t /* valid_entries_num */) {
: return true;
: };
> These seem more expressive than they need to be for the purpose of invalida
The constructor of InvalidationControl uses these.
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc@63
PS2, Line 63: const Cache::ValidityFunc Cache::kInvalidateAllEntriesFunc = [](
: Slice /* key */, Slice /* value */) {
: return false;
: };
:
: const Cache::IterationFunc Cache::kIterateOverAllEntriesFunc = [](
: size_t /* valid_entries_num */, size_t /* valid_entries_num */) {
: return true;
: };
> I was still somewhat fuzzy about the use of "invalid_entries_num", and of "
Some of that is now implemented in TTLCache, its tests and how the TTLCache is used in SentryPrivilegesFetcher.
Done.
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc@63
PS2, Line 63: const Cache::ValidityFunc Cache::kInvalidateAllEntriesFunc = [](
: Slice /* key */, Slice /* value */) {
: return false;
: };
:
: const Cache::IterationFunc Cache::kIterateOverAllEntriesFunc = [](
: size_t /* valid_entries_num */, size_t /* valid_entries_num */) {
: return true;
: };
> Ah, looking at https://gerrit.cloudera.org/c/13201/5/src/kudu/util/ttl_cach
Right.
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 01:37:32 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 1: Verified+1
unrelated flakes in Java tests:
org.apache.kudu.backup.TestKuduBackup
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 30 Apr 2019 20:36:20 +0000
Gerrit-HasComments: No
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@243
PS2, Line 243: from the oldest (less relevant) entries to the newest (more relevant) ones.
> Thanks for the explanation. This comment doesn't quite reflect that I think
Yep, I think it will make it clear. I'll add this along with the change on the signature of the cache. Thanks for pointing at that!
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 20:52:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
Small nit, feel free to ignore if you disagree
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@243
PS2, Line 243: from the oldest (less relevant) entries to the newest (more relevant) ones.
> Caches with both LRU and FIFO eviction policies keep their lists organized
Thanks for the explanation. This comment doesn't quite reflect that I think; for an LRU policy, the best candidate isn't the oldest, but rather, the least-recently used. Perhaps reword it slightly:
"The invalidation process iterates over the cache's recency list(s), from best candidate for eviction to the worst." or somesuch?
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 02 May 2019 19:08:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 1:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG@7
PS1, Line 7: [util] interface to invalidate cache entries
What motivated this patch? The CLI tool outright reset the entire TTL cache; is this an alternative to that approach?
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@246
PS1, Line 246: typedef std::function<bool(size_t /* valid_entries_num */,
I think it'd be simpler to combine this into the ValidityFunc. Invalidate callers that aren't interested in early out can ignore these two args. You can use an enum (or out param) to represent all of the "return states" of the functor.
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@251
PS1, Line 251: a
an
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@252
PS1, Line 252: optional
If it's optional, perhaps wrap it in boost::optional?
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@255
PS1, Line 255: when the provided functor is non-null
Both functors are passed by cref so how could either be null?
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@256
PS1, Line 256: shard
Sharding is an implementation detail and not a requirement for implementing this interface. Does this mean that all implementors must be sharded? If not, please find a way to express this without implying that implementors must shard.
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@260
PS1, Line 260: virtual size_t Invalidate(const ValidityFunc& validity_func,
It's not clear from the commit message (or context) why all of this is useful:
1. Why do we need cache invalidation support?
2. Why is the short-circuit provided by IterationFunc useful?
3. Why does IterationFunc benefit from its two arguments?
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.h@268
PS1, Line 268: examing
examine
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13196/1/src/kudu/util/cache.cc@508
PS1, Line 508: std::lock_guard<MutexType> l(mutex_);
If holding the mutex_ while calling the functors is a requirement, should document that in the header file (or something to that effect i.e. the functors must do a minimal amount of work because the cache may hold an internal lock while calling them).
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Tue, 30 Apr 2019 22:34:39 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc@63
PS2, Line 63: const Cache::ValidityFunc Cache::kInvalidateAllEntriesFunc = [](
: Slice /* key */, Slice /* value */) {
: return false;
: };
:
: const Cache::IterationFunc Cache::kIterateOverAllEntriesFunc = [](
: size_t /* valid_entries_num */, size_t /* valid_entries_num */) {
: return true;
: };
> Ah, looking at https://gerrit.cloudera.org/c/13201/5/src/kudu/util/ttl_cach
I was still somewhat fuzzy about the use of "invalid_entries_num", and of "key". After re-reading the .h after looking at the next CR, I can see at least how these _could_ be used in the future. Do you intend on implementing what is described in the .h as well?
Also the second argument in kIterateOverAllEntriesFunc should be "invalid..num"
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 19:40:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@266
PS2, Line 266: For example, in case of
: // multi-shard cache, when the 'iteration_func' returns 'false',
: // the invalidation at current shard stops and switches to the next
: // non-yet-processed shard, if any is present.
An alternative would be to reduce the two numeric arguments into a single boolean that describes whether any valid entries had been encountered. That would support the TTL cache use case while reducing complexity here. What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 16:19:49 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@266
PS2, Line 266: For example, in case of
: // multi-shard cache, when the 'iteration_func' returns 'false',
: // the invalidation at current shard stops and switches to the next
: // non-yet-processed shard, if any is present.
> An alternative would be to reduce the two numeric arguments into a single b
Since it's a bit of a generic interface, I don't think that having just a boolean parameter gives a good sense on the progress of iteration over the entries, even for the case of a cache with FIFO eviction policy. E.g., if wanting to account for how many entries have been processed so far in a shard, having just the boolean parameter assumes the ProcessingControl should be stateful. I'm not sure that's the best way to go.
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 18:48:53 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13196/1//COMMIT_MSG@7
PS1, Line 7: [util] interface to invalidate cache entries
> What motivated this patch? The CLI tool outright reset the entire TTL cache
Answering my own question: this supports the TTL cache scrubbing thread. Could you doc that?
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 05:24:24 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
[kudu-CR] [util] interface to invalidate cache entries
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13196 )
Change subject: [util] interface to invalidate cache entries
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@243
PS2, Line 243: from the oldest (less relevant) entries to the newest (more relevant) ones.
Does the order depend on the kind of recency list it is?
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@265
PS2, Line 265: Ivalidate
Invalidate
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@266
PS2, Line 266: For example, in case of
: // multi-shard cache, when the 'iteration_func' returns 'false',
: // the invalidation at current shard stops and switches to the next
: // non-yet-processed shard, if any is present.
> Yeah but these examples don't reflect real use cases in Kudu yet, nor are s
I agree, I wasn't sure what the motivation behind these functions were, given that they aren't really used in very meaningful way today.
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.h@304
PS2, Line 304: to for
nit: for
same below
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13196/2/src/kudu/util/cache.cc@63
PS2, Line 63: const Cache::ValidityFunc Cache::kInvalidateAllEntriesFunc = [](
: Slice /* key */, Slice /* value */) {
: return false;
: };
:
: const Cache::IterationFunc Cache::kIterateOverAllEntriesFunc = [](
: size_t /* valid_entries_num */, size_t /* valid_entries_num */) {
: return true;
: };
These seem more expressive than they need to be for the purpose of invalidating all non-valid entries. Do you have something in the works that makes use of these parameters?
--
To view, visit http://gerrit.cloudera.org:8080/13196
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1e340c3270b5580114151cdd40afe82576c18099
Gerrit-Change-Number: 13196
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: Tidy Bot (241)
Gerrit-Comment-Date: Wed, 01 May 2019 19:25:04 +0000
Gerrit-HasComments: Yes