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 2017/09/23 00:18:55 UTC
[kudu-CR] periodic: add one-shot timers
Hello Alexey Serbin, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/8130
to review the following change.
Change subject: periodic: add one-shot timers
......................................................................
periodic: add one-shot timers
One-shot timers will Stop() themselves after running a user task once.
Note: it's worth pointing out that if Callback() races with Snooze(), the
outcome is somewhat undefined. That is, there's no telling whether the
currently running callback will run the task or snooze it. The same holds
for one-shot timers, albeit more profoundly, as Callback() can also race
with Start(). To avoid this, Start() the one-shot timer from within the
task, or right after the task.
I also refactored one-shot and jitter percentage into a new options struct.
Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
3 files changed, 134 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8130/1
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] periodic: add one-shot timers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Overall looks good. Are you going to address TidyBot's feedback?
http://gerrit.cloudera.org:8080/#/c/8130/3/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
http://gerrit.cloudera.org:8080/#/c/8130/3/src/kudu/rpc/periodic.cc@61
PS3, Line 61: options_(std::move(options)),
> warning: std::move of the variable 'options' of the trivially-copyable type
yep, it seems using move() for Options does not much sense.
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 04 Oct 2017 20:54:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8130/3/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
http://gerrit.cloudera.org:8080/#/c/8130/3/src/kudu/rpc/periodic.cc@61
PS3, Line 61: options_(std::move(options)),
> yep, it seems using move() for Options does not much sense.
See https://gerrit.cloudera.org/c/8130/1/src/kudu/rpc/periodic.cc#61.
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 04 Oct 2017 21:27:54 +0000
Gerrit-HasComments: Yes
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8130
to look at the new patch set (#2).
Change subject: periodic: add one-shot timers
......................................................................
periodic: add one-shot timers
One-shot timers will Stop() themselves after running a user task once.
Note: it's worth pointing out that if Callback() races with Snooze(), the
outcome is somewhat undefined. That is, there's no telling whether the
currently running callback will run the task or snooze it. The same holds
for one-shot timers, albeit more profoundly, as Callback() can also race
with Start(). To avoid this, Start() the one-shot timer from within the
task, or right after the task.
I also refactored one-shot and jitter percentage into a new options struct.
Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
3 files changed, 134 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8130/2
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
periodic: add one-shot timers
One-shot timers will Stop() themselves after running a user task once.
Note: it's worth pointing out that if Callback() races with Snooze(), the
outcome is somewhat undefined. That is, there's no telling whether the
currently running callback will run the task or snooze it. The same holds
for one-shot timers, albeit more profoundly, as Callback() can also race
with Start(). To avoid this, Start() the one-shot timer from within the
task, or right after the task.
I also refactored one-shot and jitter percentage into a new options struct.
Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Reviewed-on: http://gerrit.cloudera.org:8080/8130
Tested-by: Adar Dembo <ad...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
3 files changed, 138 insertions(+), 18 deletions(-)
Approvals:
Adar Dembo: Verified
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8130/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
http://gerrit.cloudera.org:8080/#/c/8130/2/src/kudu/rpc/periodic.h@81
PS2, Line 81: // The timer will automatically stop after running the user's task.
> can you clarify a bit what the behavior of "Snooze" and "Stop" are in the c
Yes on both counts.
I'll update the comment.
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Oct 2017 04:15:09 +0000
Gerrit-HasComments: Yes
[kudu-CR] periodic: add one-shot timers
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Patch Set 3: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8130/3/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
http://gerrit.cloudera.org:8080/#/c/8130/3/src/kudu/rpc/periodic.cc@61
PS3, Line 61: options_(std::move(options)),
> See https://gerrit.cloudera.org/c/8130/1/src/kudu/rpc/periodic.cc#61.
Oh, I missed that -- sorry.
Yes, your reasoning makes sense.
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 04 Oct 2017 21:33:48 +0000
Gerrit-HasComments: Yes
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8130
to look at the new patch set (#3).
Change subject: periodic: add one-shot timers
......................................................................
periodic: add one-shot timers
One-shot timers will Stop() themselves after running a user task once.
Note: it's worth pointing out that if Callback() races with Snooze(), the
outcome is somewhat undefined. That is, there's no telling whether the
currently running callback will run the task or snooze it. The same holds
for one-shot timers, albeit more profoundly, as Callback() can also race
with Start(). To avoid this, Start() the one-shot timer from within the
task, or right after the task.
I also refactored one-shot and jitter percentage into a new options struct.
Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
---
M src/kudu/rpc/periodic-test.cc
M src/kudu/rpc/periodic.cc
M src/kudu/rpc/periodic.h
3 files changed, 138 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/30/8130/3
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Patch Set 3: Verified+1
Overriding Jenkins, got clock synchronization errors in some of the distributed tests.
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 02 Oct 2017 16:01:30 +0000
Gerrit-HasComments: No
[kudu-CR] periodic: add one-shot timers
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8130/2/src/kudu/rpc/periodic.h
File src/kudu/rpc/periodic.h:
http://gerrit.cloudera.org:8080/#/c/8130/2/src/kudu/rpc/periodic.h@81
PS2, Line 81: // The timer will automatically stop after running the user's task.
can you clarify a bit what the behavior of "Snooze" and "Stop" are in the case of a one-shot? i.e does Snooze have the effect of continuing to postpone the firing of the one-shot, and Stop has the effect of canceling it? (assuming that it hasn't fired yet, of course)
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 29 Sep 2017 18:56:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has removed Kudu Jenkins from this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Removed reviewer Kudu Jenkins with the following votes:
* Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteReviewer
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] periodic: add one-shot timers
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8130 )
Change subject: periodic: add one-shot timers
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8130/1/src/kudu/rpc/periodic.cc
File src/kudu/rpc/periodic.cc:
http://gerrit.cloudera.org:8080/#/c/8130/1/src/kudu/rpc/periodic.cc@61
PS1, Line 61: options_(std::move(options)),
> warning: std::move of the variable 'options' of the trivially-copyable type
I'm going to ignore these warnings, because they'll be wrong if/when PeriodicTimer::Options grows a member that is a candidate for moving (like std::string), and we won't know to update all the pass-by-value sites.
--
To view, visit http://gerrit.cloudera.org:8080/8130
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4d9376172d66c92958071d5abbac63d751e41f3
Gerrit-Change-Number: 8130
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Sat, 23 Sep 2017 00:43:11 +0000
Gerrit-HasComments: Yes