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/25 01:57:26 UTC
[kudu-CR] [ttl cache] allow outstanding handles
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13112
Change subject: [ttl_cache] allow outstanding handles
......................................................................
[ttl_cache] allow outstanding handles
This patch improves TTLCache so that it's safe to keep a handle to
an entry in the cache even if the cache itself has already gone.
Added corresponding tests to cover new functionality.
Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
---
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/util/cache.h
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
4 files changed, 153 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/13112/1
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] NOT FOR COMMIT [ttl cache] allow outstanding handles
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: NOT FOR COMMIT [ttl_cache] allow outstanding handles
......................................................................
Patch Set 2: Verified+1
Unrelated flakes in Java tests:
org.apache.kudu.flume.sink.SecureKuduSinkTest
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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-Comment-Date: Fri, 26 Apr 2019 00:14:20 +0000
Gerrit-HasComments: No
[kudu-CR] [ttl cache] allow outstanding handles
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: [ttl_cache] allow outstanding handles
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG@9
PS1, Line 9: This patch improves TTLCache so that it's safe to keep a handle to
: an entry in the cache even if the cache itself has already gone.
> From the other side, adding shared pointers here and there has their costs.
Frankly, I'm finding the ownership semantics difficult to follow due to the number of layers (the sharded cache, the TTL cache, the cache handles, the TTL cache's handles), so anything that simplifies ownership is a win in my book.
And I'd definitely vote against added flexibility if there's no use case for it. You can doc the possibilities should it come up in the future.
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 17:05:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ttl cache] allow outstanding handles
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: [ttl_cache] allow outstanding handles
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG@9
PS1, Line 9: This patch improves TTLCache so that it's safe to keep a handle to
: an entry in the cache even if the cache itself has already gone.
> I think using std::atomic_exchange or whatever to get a copy of shared_ptr
From the other side, adding shared pointers here and there has their costs. I think I'll update this patch to have a special flavor of TTLCache that allows for outstanding handles (seems to be feasible using additional template parameter and enable-if meta-magic).
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 16:52:31 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ttl cache] allow outstanding handles
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: [ttl_cache] allow outstanding handles
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG@9
PS1, Line 9: This patch improves TTLCache so that it's safe to keep a handle to
: an entry in the cache even if the cache itself has already gone.
> In https://gerrit.cloudera.org/c/13063/2//COMMIT_MSG#22, I did suggest that
I think using std::atomic_exchange or whatever to get a copy of shared_ptr so that the original might be reset under the hood is the way to go for 13063.
However, I think it's nice to have a TTLCache that allows for having outstanding handles for its entries. What do you think?
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 06:01:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ttl cache] allow outstanding handles
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: [ttl_cache] allow outstanding handles
......................................................................
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG@9
PS1, Line 9: This patch improves TTLCache so that it's safe to keep a handle to
: an entry in the cache even if the cache itself has already gone.
In https://gerrit.cloudera.org/c/13063/2//COMMIT_MSG#22, I did suggest that you may need a patch like this if handles could outlive the cache. But if the cache itself has shared ownership, are you sure that isn't sufficient? AFAICT, the only place we use cache handles is in https://gerrit.cloudera.org/c/13063/2/src/kudu/master/sentry_privileges_fetcher.cc#453, and it seems really easy to declare a local shared_ptr that'll take a ref on cache_ before 'handle' such that the latter is destroyed before the former.
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/cache.h@55
PS1, Line 55: references left elsewhere
Nit: "references are left elsewhere".
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache.h
File src/kudu/util/ttl_cache.h:
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache.h@125
PS1, Line 125: is added to allow
Nit: replace with "allows"
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache.h@126
PS1, Line 126: if
Nit: drop
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache.h@129
PS1, Line 129: // assumes the cache hasn't been destroyed yet upon the destruction of
assume ("eviction/deallocation mechanics" are plural)
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 04:02:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] NOT FOR COMMIT [ttl cache] allow outstanding handles
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13112
to look at the new patch set (#2).
Change subject: NOT FOR COMMIT [ttl_cache] allow outstanding handles
......................................................................
NOT FOR COMMIT [ttl_cache] allow outstanding handles
********************
NOTE
This changelist does not bring much value unless there is a use case
where TTLCache is used such a way that it's necessary
to allow for outstanding handles.
********************
This patch improves TTLCache so that it's safe to keep a handle to
an entry in the cache even if the cache itself has already gone.
Added corresponding tests to cover new functionality.
Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
---
M src/kudu/master/sentry_privileges_fetcher.h
M src/kudu/util/cache.h
M src/kudu/util/ttl_cache-test.cc
M src/kudu/util/ttl_cache.h
4 files changed, 153 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/12/13112/2
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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)
[kudu-CR] NOT FOR COMMIT [ttl cache] allow outstanding handles
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: NOT FOR COMMIT [ttl_cache] allow outstanding handles
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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>
[kudu-CR] [ttl cache] allow outstanding handles
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: [ttl_cache] allow outstanding handles
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache-test.cc@375
PS1, Line 375: a entry
an entry
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache-test.cc@407
PS1, Line 407: TEST_F(TTLCacheTest, CacheAndOutstandingHandles) {
Is this test a superset of CacheAndOutstandingHandlesNoMetrics? Should we just keep this one? Or is the no-metrics version easier to debug (and therefore still valuable)?
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 18:07:13 +0000
Gerrit-HasComments: Yes
[kudu-CR] [ttl cache] allow outstanding handles
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13112 )
Change subject: [ttl_cache] allow outstanding handles
......................................................................
Patch Set 1:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG
Commit Message:
PS1:
This changelist does not bring much value unless there is a use case where TTLCache is used such a way that it's necessary to allow for outstanding handles.
As of now, I'll keep aside.
Thank you all for the feedback!
http://gerrit.cloudera.org:8080/#/c/13112/1//COMMIT_MSG@9
PS1, Line 9: This patch improves TTLCache so that it's safe to keep a handle to
: an entry in the cache even if the cache itself has already gone.
> Frankly, I'm finding the ownership semantics difficult to follow due to the
SGTM. That makes sense, especially given the tight timeline we have for the integration testing of Kudu+HMS+Sentry.
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache-test.cc
File src/kudu/util/ttl_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13112/1/src/kudu/util/ttl_cache-test.cc@407
PS1, Line 407: TEST_F(TTLCacheTest, CacheAndOutstandingHandles) {
> Is this test a superset of CacheAndOutstandingHandlesNoMetrics? Should we j
The initial idea was to check whether there are any problems even with non-metricised instances. The case with metrics could fail while accessing invalid metrics pointer during eviction callback, so I looked elsewhere. Indeed, there were issues, but those were due to triggering DCHECKs in the Cache's code itself :)
In other words, the non-metricised scenario was useful for the exploration phase.
Right, if going forward with this changelist, probably it makes sense to drop the non-metricised scenario this this one is a super-set of the former, as you noticed.
--
To view, visit http://gerrit.cloudera.org:8080/13112
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I280884a630985c5783b7f99f992f530b5f4b9d50
Gerrit-Change-Number: 13112
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Thu, 25 Apr 2019 21:57:47 +0000
Gerrit-HasComments: Yes