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/29 15:42:46 UTC
[kudu-CR] [util] modernize signature of Cache interface methods
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13175
Change subject: [util] modernize signature of Cache interface methods
......................................................................
[util] modernize signature of Cache interface methods
Updated the signature of the Lookup() and Insert() methods of the Cache
interface to return UniqueHandle. That helps in automating reference
counting for the cached items.
Also, Release() and Free() methods have been moved into the protected
section. With UniqueHandle returned from Lookup() and Insert(),
Release() and Free() are no longer required to be the public part
of the Cache's interface.
Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/block_handle.h
M src/kudu/codegen/code_cache.cc
M src/kudu/util/cache-bench.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
12 files changed, 150 insertions(+), 179 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/13175/1
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@101
PS2, Line 101: typedef std::unique_ptr<Handle, HandleDeleter> UniqueHandle;
At this point I'm not sure whether it makes sense to continue with using std::unique_ptr wrapper around an opaque structure or just introduce a handle class on its own.
I'm keeping this as is for now.
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Mon, 29 Apr 2019 23:01:33 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Tue, 07 May 2019 05:11:10 +0000
Gerrit-HasComments: No
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc
File src/kudu/codegen/code_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc@83
PS2, Line 83: memcpy(cache_->MutableValue(&pending), &val, val_len);
> The idea was to hide raw handles from the Cache's interface. Using UniqueP
Ah, regarding the original question: nope, that's not an overload, that's just taking address of the std::unique_ptr instance.
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Tue, 30 Apr 2019 01:32:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Andrew Wong, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13175
to look at the new patch set (#3).
Change subject: [util] modernize signature of Cache interface methods
......................................................................
[util] modernize signature of Cache interface methods
Updated the signature of the Lookup() and Insert() methods of the Cache
interface to return UniqueHandle. That helps in automating reference
counting for the cached items.
Also, Release() and Free() methods have been moved into the protected
section. With UniqueHandle returned from Lookup() and Insert(),
Release() and Free() are no longer required to be the public part
of the Cache's interface.
Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/block_handle.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/codegen/code_cache.cc
M src/kudu/util/cache-bench.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 189 insertions(+), 212 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/13175/3
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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>
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@101
PS2, Line 101: typedef std::unique_ptr<Handle, HandleDeleter> UniqueHandle;
> I guess if we did, we'd be a be able to make HandleDeleter private? But it'
Right -- implementing a custom handle class would allow to remove mention of Handle, PendingHandle and their deleters out from this header file.
We can do it later, and for now keep std::unique_ptr typedefs.
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@150
PS2, Line 150: virtual uint8_t* MutableValue(UniquePendingHandle* handle) = 0;
> Missed this?
Done
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@150
PS2, Line 150: virtual uint8_t* MutableValue(UniquePendingHandle* handle) = 0;
> FWIW, this was previously near Allocate because it was part of the "inserti
Done
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.cc@553
PS2, Line 553: h
> Missed this?
Done
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.cc@553
PS2, Line 553: h
> nit: This is named "handle" in the .h file.
Done
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Tue, 07 May 2019 05:02:34 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/13175/2//COMMIT_MSG
Commit Message:
PS2:
Thanks for making this change! It makes the code much nicer to reason about IMO.
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@73
PS2, Line 73: // Sample usage:
: //
: // std::unique_ptr<Cache> cache(NewLRUCache(...));
: // ...
: // {
: // Cache::UniqueHandle h(cache->Lookup(...)));
: // ...
: // } // 'h' is automatically released here
: //
nit: perhaps this is better suited to be by L101?
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@101
PS2, Line 101: typedef std::unique_ptr<Handle, HandleDeleter> UniqueHandle;
> At this point I'm not sure whether it makes sense to continue with using st
I guess if we did, we'd be a be able to make HandleDeleter private? But it's not hurting readability AFAICT, +1 for keeping it as is.
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.cc@553
PS2, Line 553: h
nit: This is named "handle" in the .h file.
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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: Wed, 01 May 2019 18:59:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
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/13175
to look at the new patch set (#4).
Change subject: [util] modernize signature of Cache interface methods
......................................................................
[util] modernize signature of Cache interface methods
Updated the signature of the Lookup() and Insert() methods of the Cache
interface to return UniqueHandle. That helps in automating reference
counting for the cached items.
Also, Release() and Free() methods have been moved into the protected
section. With UniqueHandle returned from Lookup() and Insert(),
Release() and Free() are no longer required to be the public part
of the Cache's interface.
Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/block_handle.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/codegen/code_cache.cc
M src/kudu/util/cache-bench.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 219 insertions(+), 236 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/13175/4
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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)
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@73
PS2, Line 73:
: // Custom handle "deleter", primarily intended for use with std::unique_ptr.
: // Sample usage:
: //
: // unique_ptr<Cache> cache(NewCache(...));
: // ...
: // {
: // unique_ptr<Cache::Handle, Cache::HandleDeleter> h(
: //
> nit: perhaps this is better suited to be by L101?
Missed this?
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@150
PS2, Line 150: //
> FWIW, this was previously near Allocate because it was part of the "inserti
Missed this?
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.cc
File src/kudu/util/cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.cc@553
PS2, Line 553:
> nit: This is named "handle" in the .h file.
Missed this?
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Tue, 07 May 2019 04:00:55 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc
File src/kudu/codegen/code_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc@83
PS2, Line 83: memcpy(cache_->MutableValue(&pending), &val, val_len);
> Is this an overload? I think get() is more commonly used in Kudu to get the
The idea was to hide raw handles from the Cache's interface. Using UniquePendingHandle is an overhead on itself, and not only in this particular case, sure.
The pointer is used here since the code style doesn't allow to pass a non-cost reference as a parameter, requiring passing a pointer.
So, what's the verdict? Keep Handle and PendingHandle in the signature of methods in the Cache's interface because we don't want to incur overhead due to passing around references/pointers to UniqueHandle/UniquePendingHandle?
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Mon, 29 Apr 2019 23:22:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2: Verified+1
unrelated flake in:
* org.apache.kudu.client.TestTimeouts
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Mon, 29 Apr 2019 23:29:10 +0000
Gerrit-HasComments: No
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/13175/2//COMMIT_MSG
Commit Message:
PS2:
> Thanks for making this change! It makes the code much nicer to reason about
Thank you for the review!
That's the change suggested by Adar some time ago: https://gerrit.cloudera.org/#/c/12876/1/src/kudu/util/cache.h@225
I'll address the feedback once the changes for Cache::Invalidate() land, since those conflict with this one.
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc
File src/kudu/codegen/code_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc@83
PS2, Line 83: memcpy(cache_->MutableValue(&pending), &val, val_len);
> Right, I misunderstood and forgot MutableValue and friends expected pointer
I don't think there is an overhead passing references and pointers to std::unique_ptr around comparing with passing just raw pointers to Cache::Handle. I looked into the <memory> header file and didn't find any overloads for the '&' operator. So, from that perspective changing the signature of Cache's methods this way looks good to me.
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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: Thu, 02 May 2019 01:46:23 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc
File src/kudu/codegen/code_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc@83
PS2, Line 83: memcpy(cache_->MutableValue(&pending), &val, val_len);
Right, I misunderstood and forgot MutableValue and friends expected pointers to the std::unique_ptr.
> So, what's the verdict? Keep Handle and PendingHandle in the signature of methods in the Cache's interface because we don't want to incur overhead due to passing around references/pointers to UniqueHandle/UniquePendingHandle?
You tell me. :) Is there overhead to passing them around? I don't believe there is, but maybe they overload their operators in a surprising way.
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Wed, 01 May 2019 05:26:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13175/1//COMMIT_MSG
Commit Message:
PS1:
It seems some tests fail. I'll take a closer look.
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Mon, 29 Apr 2019 17:04:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc
File src/kudu/codegen/code_cache.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/codegen/code_cache.cc@83
PS2, Line 83: memcpy(cache_->MutableValue(&pending), &val, val_len);
Is this an overload? I think get() is more commonly used in Kudu to get the address of an object inside a unique_ptr, no?
Applies to several other changes.
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h
File src/kudu/util/cache.h:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/cache.h@150
PS2, Line 150: virtual uint8_t* MutableValue(UniquePendingHandle* handle) = 0;
FWIW, this was previously near Allocate because it was part of the "insertion path" (see block comment below).
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/file_cache-test.cc
File src/kudu/util/file_cache-test.cc:
http://gerrit.cloudera.org:8080/#/c/13175/2/src/kudu/util/file_cache-test.cc@169
PS2, Line 169: auto uh(this->cache_->cache_->Lookup(kFile1, Cache::EXPECT_IN_CACHE));
: ASSERT_FALSE(uh);
Nit: could combine? Then you could eliminate the new scopes.
Above too.
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Mon, 29 Apr 2019 23:04:58 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
[util] modernize signature of Cache interface methods
Updated the signature of the Lookup() and Insert() methods of the Cache
interface to return UniqueHandle. That helps in automating reference
counting for the cached items.
Also, Release() and Free() methods have been moved into the protected
section. With UniqueHandle returned from Lookup() and Insert(),
Release() and Free() are no longer required to be the public part
of the Cache's interface.
Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Reviewed-on: http://gerrit.cloudera.org:8080/13175
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/block_handle.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/codegen/code_cache.cc
M src/kudu/util/cache-bench.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 219 insertions(+), 236 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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>
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/13175
to look at the new patch set (#2).
Change subject: [util] modernize signature of Cache interface methods
......................................................................
[util] modernize signature of Cache interface methods
Updated the signature of the Lookup() and Insert() methods of the Cache
interface to return UniqueHandle. That helps in automating reference
counting for the cached items.
Also, Release() and Free() methods have been moved into the protected
section. With UniqueHandle returned from Lookup() and Insert(),
Release() and Free() are no longer required to be the public part
of the Cache's interface.
Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
---
M src/kudu/cfile/block_cache.cc
M src/kudu/cfile/block_cache.h
M src/kudu/cfile/block_handle.h
M src/kudu/cfile/cfile-test.cc
M src/kudu/codegen/code_cache.cc
M src/kudu/util/cache-bench.cc
M src/kudu/util/cache-test.cc
M src/kudu/util/cache.cc
M src/kudu/util/cache.h
M src/kudu/util/file_cache-test.cc
M src/kudu/util/file_cache.cc
M src/kudu/util/nvm_cache.cc
M src/kudu/util/ttl_cache.h
13 files changed, 180 insertions(+), 213 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/75/13175/2
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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)
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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>
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] modernize signature of Cache interface methods
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13175 )
Change subject: [util] modernize signature of Cache interface methods
......................................................................
Patch Set 4: Verified+1
flakiness in unrelated Java tests:
* org.apache.kudu.mapreduce.tools.ITImportCsv
* org.apache.kudu.client.TestHandleTooBusy
* org.apache.kudu.client.ITFaultTolerantScanner
--
To view, visit http://gerrit.cloudera.org:8080/13175
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I31518cdfa269e6806aa5dda180956896d8cb3404
Gerrit-Change-Number: 13175
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-Comment-Date: Tue, 07 May 2019 05:51:50 +0000
Gerrit-HasComments: No