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/11 19:07:47 UTC
[kudu-CR] [util] separate generalized CacheMetrics
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12716
Change subject: [util] separate generalized CacheMetrics
......................................................................
[util] separate generalized CacheMetrics
Separated the generic counters into CacheMetrics to make it possible
using CacheMetrics for other cache types. The LRUCache now uses
LRUCacheMetrics which has some specifics attributed to LRUCache itself.
Also, the instantiations of metrics counters specific for the block LRU
cache have been moved into BlockCacheMetrics structure.
These changes are introduced to accommodate a few follow-up
modifications introducing a TTL-based cache.
Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
---
M src/kudu/cfile/block_cache.cc
M src/kudu/util/CMakeLists.txt
R src/kudu/util/block_cache_metrics.cc
A src/kudu/util/block_cache_metrics.h
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/cache_metrics.h
M src/kudu/util/file_cache.cc
A src/kudu/util/lru_cache_metrics.h
M src/kudu/util/nvm_cache.cc
11 files changed, 120 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/12716/1
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] separate generalized CacheMetrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12716 )
Change subject: [util] separate generalized CacheMetrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG@15
PS1, Line 15: These changes are introduced to accommodate a few follow-up
: modifications introducing a TTL-based cache.
I'm still not convinced that a separate base implementation for the TTL-based cache is better than reusing the one in util/cache.{cc,h}. Can you share more details on what challenges you faced?
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Mon, 11 Mar 2019 19:16:59 +0000
Gerrit-HasComments: Yes
[kudu-CR] DO NOT REVIEW [util] separate generalized CacheMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has abandoned this change. ( http://gerrit.cloudera.org:8080/12716 )
Change subject: DO NOT REVIEW [util] separate generalized CacheMetrics
......................................................................
Abandoned
obsoleted by https://gerrit.cloudera.org/#/c/12776/
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
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)
[kudu-CR] [util] separate generalized CacheMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12716 )
Change subject: [util] separate generalized CacheMetrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG@15
PS1, Line 15: These changes are introduced to accommodate a few follow-up
: modifications introducing a TTL-based cache.
> I didn't try to implement that cache using cache.{h,cc} -- I abandoned that
'... The LRU cache tends to cache usage in memory ...' -- I meant 'The LRU cache tends to _report_ cache usage in memory _units_, ...'
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
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: Mon, 11 Mar 2019 21:53:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] separate generalized CacheMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12716 )
Change subject: [util] separate generalized CacheMetrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG@15
PS1, Line 15: These changes are introduced to accommodate a few follow-up
: modifications introducing a TTL-based cache.
> I'm still not convinced that a separate base implementation for the TTL-bas
I didn't try to implement that cache using cache.{h,cc} -- I abandoned that though right after looking at interface in cache.h and reading of optimizations to achieve 5% optimizations for gcc 4.4.x.
My points after looking at that LRU cache implementation are:
1) The interface doesn't seem simple enough for the simple task I want to accomplish, e.g. using Allocate(), using Slice for parameters, wrappers for Handles, sounds too much for this simple thing.
2) The LRU cache tends to cache usage in memory, while for the entries I was thinking to cache I'm not sure there is a good estimation for memory usage (apache::thrift::TBase-derived sentry::TListSentryPrivilegesResponse). Instead, I'm thinking to report the usage of that cache as number of items.
If you still think adopting LRU cache for that is a good idea, I can go try LRU cache for that.
BTW, I think this changelist will be needed even if going along the 'use the LRU-cache for all' route because it's necessary to separate instantiating metrics for various caches at least to have proper descriptions for the metric entities (former cache_metrics.{cc,h} were all about kBytes and block cache).
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
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: Mon, 11 Mar 2019 21:42:20 +0000
Gerrit-HasComments: Yes
[kudu-CR] DO NOT REVIEW [util] separate generalized CacheMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/12716
to look at the new patch set (#2).
Change subject: DO NOT REVIEW [util] separate generalized CacheMetrics
......................................................................
DO NOT REVIEW [util] separate generalized CacheMetrics
Separated the generic counters into CacheMetrics to make it possible
using CacheMetrics for other cache types. The LRUCache now uses
LRUCacheMetrics which has some specifics attributed to LRUCache itself.
Also, the instantiations of metrics counters specific for the block LRU
cache have been moved into BlockCacheMetrics structure.
These changes are introduced to accommodate a few follow-up
modifications introducing a TTL-based cache.
Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
---
M src/kudu/cfile/block_cache.cc
M src/kudu/util/CMakeLists.txt
R src/kudu/util/block_cache_metrics.cc
A src/kudu/util/block_cache_metrics.h
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/cache_metrics.h
M src/kudu/util/file_cache.cc
A src/kudu/util/lru_cache_metrics.h
M src/kudu/util/nvm_cache.cc
11 files changed, 120 insertions(+), 48 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/12716/2
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
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)
[kudu-CR] [util] separate generalized CacheMetrics
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12716 )
Change subject: [util] separate generalized CacheMetrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG@15
PS1, Line 15: These changes are introduced to accommodate a few follow-up
: modifications introducing a TTL-based cache.
> '... The LRU cache tends to cache usage in memory ...' -- I meant 'The LRU
Regarding #1, I agree that the implementation has some rough edges, though users of a TTL-based cache need not see that (see FileCache for an example as to how that could be hidden).
Regarding #2, I'm concerned that if TTL is the only thing expiring cached objects, our memory footprint may grow without bound based on how an external service is configured. Meaning, I'd be more comfortable if we used TTL _and_ memory as bounds for the cache. And to do that, you'll need something like util/cache.{h,cc}. Unless you can show me that this isn't a legitimate concern, that Sentry won't be able to inadvertently DoS the master in this way.
It's frustrating that Thrift-based objects don't provide methods that measure memory footprint, but if all we're caching is TListSentryPrivilegesResponse (or TSentryPrivilege, or whatever), we can measure it ourselves. Looking at the generated code, a malloc_usable_size(ptr) call plus some string capacity calls should do the trick. Look at PosixSequentialFile::memory_footprint() for an example.
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
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: Tue, 12 Mar 2019 01:51:01 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] separate generalized CacheMetrics
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12716 )
Change subject: [util] separate generalized CacheMetrics
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/12716/1//COMMIT_MSG@15
PS1, Line 15: These changes are introduced to accommodate a few follow-up
: modifications introducing a TTL-based cache.
> Regarding #1, I agree that the implementation has some rough edges, though
Thanks -- that makes sense. I especially don't want to keep Kudu master open for DoS-like issues caused by huge Sentry responses. I agree that putting capacity on just number of items is the cache is not enough in that case since I realized there is no limit on the size of Sentry's response.
I'll take a closer look at adapting the LRU cache for the master authz cache needs. Thanks for the PosixSequentialFile::memory_footprint() pointer :)
--
To view, visit http://gerrit.cloudera.org:8080/12716
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iee18214b57e4e0c86e23c3b2e9cc2ee481812844
Gerrit-Change-Number: 12716
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: Tue, 12 Mar 2019 05:27:23 +0000
Gerrit-HasComments: Yes