You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2020/03/27 20:17:42 UTC
[kudu-CR] log: pass append callback by value and std::move
Hello Andrew Wong,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/15571
to review the following change.
Change subject: log: pass append callback by value and std::move
......................................................................
log: pass append callback by value and std::move
I also verified that callbacks are always provided, allowing us to remove
the various "if callback exists" checks.
Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
2 files changed, 13 insertions(+), 26 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15571/1
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Patch Set 5: Verified+1
Overriding Jenkins, test flake.
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@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: Sat, 28 Mar 2020 08:13:13 +0000
Gerrit-HasComments: No
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15571/4/src/kudu/consensus/log.h
File src/kudu/consensus/log.h:
http://gerrit.cloudera.org:8080/#/c/15571/4/src/kudu/consensus/log.h@601
PS4, Line 601: if (append_status_.ok()) {
: append_status_ = s;
: }
Do we ever expect it this method to be called twice? If not, maybe add some DCHECK constraint to track such cases at least in debug builds?
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@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: Sat, 28 Mar 2020 04:37:02 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15571
to look at the new patch set (#5).
Change subject: log: clean up entry batch append callback usage
......................................................................
log: clean up entry batch append callback usage
I started with the desire to pass these callbacks by value and move them,
then ended up doing some additional cleanup.
Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
4 files changed, 74 insertions(+), 89 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15571/5
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@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] log: clean up entry batch append callback usage
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15571/2/src/kudu/consensus/log.h
File src/kudu/consensus/log.h:
http://gerrit.cloudera.org:8080/#/c/15571/2/src/kudu/consensus/log.h@444
PS2, Line 444: Creates a new LogEntryBatch from a corresponding Protobuf message.
nit: maybe, mention the role of the 'callback' parameter?
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 2
Gerrit-Owner: 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: Sat, 28 Mar 2020 00:52:45 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/15571/4/src/kudu/consensus/log.h
File src/kudu/consensus/log.h:
http://gerrit.cloudera.org:8080/#/c/15571/4/src/kudu/consensus/log.h@600
PS4, Line 600: SetAppendStatus
> nit: maybe call this SetAppendError() and check on not OK? That'd make it c
Done
http://gerrit.cloudera.org:8080/#/c/15571/4/src/kudu/consensus/log.h@601
PS4, Line 601: if (append_status_.ok()) {
: append_status_ = s;
: }
> Do we ever expect it this method to be called twice? If not, maybe add som
It could be called twice; see Log::AppendThread::HandleBatches. If WriteBatch fails, and then Sync fails, we'll wind up calling this method again with a second error.
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@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: Sat, 28 Mar 2020 06:39:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
log: clean up entry batch append callback usage
I started with the desire to pass these callbacks by value and move them,
then ended up doing some additional cleanup.
Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Reviewed-on: http://gerrit.cloudera.org:8080/15571
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
4 files changed, 74 insertions(+), 89 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Adar Dembo: Verified
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@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] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15571/2/src/kudu/consensus/log.h
File src/kudu/consensus/log.h:
http://gerrit.cloudera.org:8080/#/c/15571/2/src/kudu/consensus/log.h@444
PS2, Line 444:
> nit: maybe, mention the role of the 'callback' parameter?
Done
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@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: Sat, 28 Mar 2020 01:00:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15571
to look at the new patch set (#3).
Change subject: log: clean up entry batch append callback usage
......................................................................
log: clean up entry batch append callback usage
I started with the desire to pass these callbacks by value and move them,
then ended up doing some additional cleanup.
Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
4 files changed, 69 insertions(+), 89 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15571/3
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 3
Gerrit-Owner: 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] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@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] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15571
to look at the new patch set (#2).
Change subject: log: clean up entry batch append callback usage
......................................................................
log: clean up entry batch append callback usage
I started with the desire to pass these callbacks by value and move them,
then ended up doing some additional cleanup.
Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
4 files changed, 68 insertions(+), 89 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15571/2
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/15571
to look at the new patch set (#4).
Change subject: log: clean up entry batch append callback usage
......................................................................
log: clean up entry batch append callback usage
I started with the desire to pass these callbacks by value and move them,
then ended up doing some additional cleanup.
Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
---
M src/kudu/consensus/log.cc
M src/kudu/consensus/log.h
M src/kudu/consensus/log_util.cc
M src/kudu/consensus/log_util.h
4 files changed, 73 insertions(+), 89 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/71/15571/4
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 4
Gerrit-Owner: 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] log: clean up entry batch append callback usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
http://gerrit.cloudera.org:8080/#/c/15571/4/src/kudu/consensus/log.h
File src/kudu/consensus/log.h:
http://gerrit.cloudera.org:8080/#/c/15571/4/src/kudu/consensus/log.h@600
PS4, Line 600: SetAppendStatus
nit: maybe call this SetAppendError() and check on not OK? That'd make it clear that the callback can be run without having called this.
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@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: Sat, 28 Mar 2020 04:32:35 +0000
Gerrit-HasComments: Yes
[kudu-CR] log: clean up entry batch append callback usage
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15571 )
Change subject: log: clean up entry batch append callback usage
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/15571
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34bbe51fbe824745fc2705819546ddf14714da52
Gerrit-Change-Number: 15571
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@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: Sat, 28 Mar 2020 06:44:37 +0000
Gerrit-HasComments: No