You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2018/06/26 01:49:44 UTC
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Hello Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10821
to review the following change.
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
consensus queue: Clarify return value of ResponseFromPeer()
This patch simply makes some minor semantic improvmenets to the
ConsensusQueue::ResponseFromPeer() API by returning the value of whether
to immediately send anothe request and improving the header file comment
documentation about those semantics.
Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
4 files changed, 85 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/10821/1
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/10821 )
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
Patch Set 2:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/10821/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10821/1//COMMIT_MSG@9
PS1, Line 9: improvmenets
> improvements
Done
http://gerrit.cloudera.org:8080/#/c/10821/1//COMMIT_MSG@11
PS1, Line 11: anothe
> another
Done
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc@528
PS1, Line 528:
: bool send_more_immediately;
> nit: combine definition and assignment
Done
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc@713
PS1, Line 713: SetLastReceivedAndLas
> Ah, it seems it has been addressed in PS2 already.
Ack
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue.cc@1033
PS1, Line 1033: bool send_more_immediately;
> nit: set to 'false' to use in the closest return:
Done
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue.cc@1043
PS1, Line 1043: return false
> nit:
Done
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 26 Jun 2018 17:59:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10821 )
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc@713
PS1, Line 713: SetLastReceivedAndLas
> Indeed: is there any assertion needed for send_more_immediately here?
Ah, it seems it has been addressed in PS2 already.
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 26 Jun 2018 03:41:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10821
to look at the new patch set (#2).
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
consensus queue: Clarify return value of ResponseFromPeer()
This patch simply makes some minor semantic improvmenets to the
ConsensusQueue::ResponseFromPeer() API by returning the value of whether
to immediately send anothe request and improving the header file comment
documentation about those semantics.
Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
4 files changed, 88 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/10821/2
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 2
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10821
to look at the new patch set (#3).
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
consensus queue: Clarify return value of ResponseFromPeer()
This patch simply makes some minor semantic improvements to the
ConsensusQueue::ResponseFromPeer() API by returning the value of whether
to immediately send another request and improving the header file comment
documentation about those semantics.
Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
4 files changed, 89 insertions(+), 79 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/21/10821/3
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10821 )
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
Patch Set 1:
(6 comments)
looks good, just few nits
http://gerrit.cloudera.org:8080/#/c/10821/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/10821/1//COMMIT_MSG@9
PS1, Line 9: improvmenets
improvements
http://gerrit.cloudera.org:8080/#/c/10821/1//COMMIT_MSG@11
PS1, Line 11: anothe
another
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc
File src/kudu/consensus/consensus_queue-test.cc:
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc@528
PS1, Line 528: bool send_more_immediately;
: send_more_immediately = queue_->ResponseFromPeer(response.responder_uuid(), response);
nit: combine definition and assignment
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue-test.cc@713
PS1, Line 713: send_more_immediately
Indeed: is there any assertion needed for send_more_immediately here?
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue.cc
File src/kudu/consensus/consensus_queue.cc:
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue.cc@1033
PS1, Line 1033: bool send_more_immediately;
nit: set to 'false' to use in the closest return:
bool send_more_immediately = false;
http://gerrit.cloudera.org:8080/#/c/10821/1/src/kudu/consensus/consensus_queue.cc@1043
PS1, Line 1043: return false
nit:
return send_mode_immediately;
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 1
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 26 Jun 2018 03:38:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10821 )
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
consensus queue: Clarify return value of ResponseFromPeer()
This patch simply makes some minor semantic improvements to the
ConsensusQueue::ResponseFromPeer() API by returning the value of whether
to immediately send another request and improving the header file comment
documentation about those semantics.
Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Reviewed-on: http://gerrit.cloudera.org:8080/10821
Tested-by: Kudu Jenkins
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/consensus/consensus_peers.cc
M src/kudu/consensus/consensus_queue-test.cc
M src/kudu/consensus/consensus_queue.cc
M src/kudu/consensus/consensus_queue.h
4 files changed, 89 insertions(+), 79 deletions(-)
Approvals:
Kudu Jenkins: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 4
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
[kudu-CR] consensus queue: Clarify return value of ResponseFromPeer()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/10821 )
Change subject: consensus queue: Clarify return value of ResponseFromPeer()
......................................................................
Patch Set 3: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10821
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe73b5c69630898327ac7787e3dfcb0b761f323e
Gerrit-Change-Number: 10821
Gerrit-PatchSet: 3
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Comment-Date: Tue, 26 Jun 2018 18:53:22 +0000
Gerrit-HasComments: No