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/18 23:08:16 UTC
[kudu-CR](branch-1.9.x) [master] introduce cache for location mapping assignments
Hello Will Berkeley, Kudu Jenkins,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/12783
to review the following change.
Change subject: [master] introduce cache for location mapping assignments
......................................................................
[master] introduce cache for location mapping assignments
This changelist adds a very primitive cache for location assignments.
The cache does not prevent running multiple commands for the same
location key if an entry is not present in the cache.
A unit test is also added.
Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Reviewed-on: http://gerrit.cloudera.org:8080/12634
Reviewed-by: Will Berkeley <wd...@gmail.com>
Tested-by: Kudu Jenkins
(cherry picked from commit ae6bbcaabd20955119f1d945d276589538dae928)
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/location_cache-test.cc
A src/kudu/master/location_cache.cc
A src/kudu/master/location_cache.h
4 files changed, 421 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/12783/1
--
To view, visit http://gerrit.cloudera.org:8080/12783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.9.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Gerrit-Change-Number: 12783
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR](branch-1.9.x) [master] introduce cache for location mapping assignments
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12783 )
Change subject: [master] introduce cache for location mapping assignments
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc
File src/kudu/master/location_cache.cc:
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc@130
PS1, Line 130: Status s = Subprocess::Call(argv, /*stdin_in=*/"", &location_temp, &stderr);
> What does this comment /*stdin_in=*/ mean here?
Most arguments are self explanatory based on their variable names. In cases where that isn't the case (e.g. passing in nullptr, empty string, etc.), to clarify the callsite, we generally annotate with comments like these indicating what the parameter name is.
E.g. from the context here, I can infer that this Subprocess::Call() is doing something like calling argv with nothing as input, and getting out the location (probably as stdout) and some stderr. Otherwise I'd be confused about what this "" is, without looking into the .h file.
--
To view, visit http://gerrit.cloudera.org:8080/12783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.9.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Gerrit-Change-Number: 12783
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 19 Mar 2019 21:28:16 +0000
Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) [master] introduce cache for location mapping assignments
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12783 )
Change subject: [master] introduce cache for location mapping assignments
......................................................................
[master] introduce cache for location mapping assignments
This changelist adds a very primitive cache for location assignments.
The cache does not prevent running multiple commands for the same
location key if an entry is not present in the cache.
A unit test is also added.
Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Reviewed-on: http://gerrit.cloudera.org:8080/12634
Reviewed-by: Will Berkeley <wd...@gmail.com>
Tested-by: Kudu Jenkins
(cherry picked from commit ae6bbcaabd20955119f1d945d276589538dae928)
Reviewed-on: http://gerrit.cloudera.org:8080/12783
Reviewed-by: Andrew Wong <aw...@cloudera.com>
---
M src/kudu/master/CMakeLists.txt
A src/kudu/master/location_cache-test.cc
A src/kudu/master/location_cache.cc
A src/kudu/master/location_cache.h
4 files changed, 421 insertions(+), 0 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/12783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.9.x
Gerrit-MessageType: merged
Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Gerrit-Change-Number: 12783
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR](branch-1.9.x) [master] introduce cache for location mapping assignments
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/12783 )
Change subject: [master] introduce cache for location mapping assignments
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc
File src/kudu/master/location_cache.cc:
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc@130
PS1, Line 130: Status s = Subprocess::Call(argv, /*stdin_in=*/"", &location_temp, &stderr);
> Most arguments are self explanatory based on their variable names. In cases
Thank you for taking time to explain this, Andrew!
--
To view, visit http://gerrit.cloudera.org:8080/12783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.9.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Gerrit-Change-Number: 12783
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 20 Mar 2019 03:22:16 +0000
Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) [master] introduce cache for location mapping assignments
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12783 )
Change subject: [master] introduce cache for location mapping assignments
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/12783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.9.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Gerrit-Change-Number: 12783
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 19 Mar 2019 17:44:37 +0000
Gerrit-HasComments: No
[kudu-CR](branch-1.9.x) [master] introduce cache for location mapping assignments
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12783 )
Change subject: [master] introduce cache for location mapping assignments
......................................................................
Patch Set 2: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc
File src/kudu/master/location_cache.cc:
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc@130
PS1, Line 130: Status s = Subprocess::Call(argv, /*stdin_in=*/"", &location_temp, &stderr);
> Most arguments are self explanatory based on their variable names. In cases
Ack
--
To view, visit http://gerrit.cloudera.org:8080/12783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.9.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Gerrit-Change-Number: 12783
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 20 Mar 2019 18:38:16 +0000
Gerrit-HasComments: Yes
[kudu-CR](branch-1.9.x) [master] introduce cache for location mapping assignments
Posted by "Greg Solovyev (Code Review)" <ge...@cloudera.org>.
Greg Solovyev has posted comments on this change. ( http://gerrit.cloudera.org:8080/12783 )
Change subject: [master] introduce cache for location mapping assignments
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc
File src/kudu/master/location_cache.cc:
http://gerrit.cloudera.org:8080/#/c/12783/1/src/kudu/master/location_cache.cc@130
PS1, Line 130: Status s = Subprocess::Call(argv, /*stdin_in=*/"", &location_temp, &stderr);
What does this comment /*stdin_in=*/ mean here?
--
To view, visit http://gerrit.cloudera.org:8080/12783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: branch-1.9.x
Gerrit-MessageType: comment
Gerrit-Change-Id: Icb5c436c9433acd87c44c4d81982420f33ebb4a4
Gerrit-Change-Number: 12783
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Greg Solovyev <gs...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 19 Mar 2019 18:56:10 +0000
Gerrit-HasComments: Yes