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/05/07 02:21:32 UTC
[kudu-CR] [util] introduced Synchronizer::WaitUntil()
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13255
Change subject: [util] introduced Synchronizer::WaitUntil()
......................................................................
[util] introduced Synchronizer::WaitUntil()
Added Synchronizer::WaitUntil() method to avoid converting MonoTime
into MonoDelta when the deadline is specified as MonoTime.
Updated corresponding tests for Synchronizer as well.
Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
---
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 83 insertions(+), 24 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/13255/1
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
[util] introduce Synchronizer::WaitUntil()
Added Synchronizer::WaitUntil() method to avoid converting MonoTime
into MonoDelta when the deadline is specified as MonoTime.
Updated corresponding tests for Synchronizer as well.
Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Reviewed-on: http://gerrit.cloudera.org:8080/13255
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Tested-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 76 insertions(+), 27 deletions(-)
Approvals:
Andrew Wong: Looks good to me, approved
Alexey Serbin: Verified
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
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)
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
Patch Set 1:
(2 comments)
> (2 comments)
>
> LGTM, just a couple test suggestions that I don't feel _that_
> strongly about, but would be nice. WDYT?
Thank you for the feedback! Yes, that makes sense.
http://gerrit.cloudera.org:8080/#/c/13255/1/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/13255/1/src/kudu/util/async_util-test.cc@103
PS1, Line 103: TEST_F(AsyncUtilTest, TestSynchronizerTimedWaitFor) {
> nit: would be nice if we didn't duplicate this logic. Could we perhaps para
Done
http://gerrit.cloudera.org:8080/#/c/13255/1/src/kudu/util/async_util-test.cc@105
PS1, Line 105: thread waiter;
: SCOPED_CLEANUP({
: // Waiting on the thread gives TSAN to check that no thread safety issues
: // occurred.
: waiter.join();
> nit: maybe pull this out of scope and keep the old L113 waiter.join()? That
I just separated these scenarios logically, and that issue has gone.
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
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: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Sat, 11 May 2019 01:12:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
Patch Set 2: Verified+1
unrelated flake in TabletHistoryGcITest.TestSnapshotScanBeforeAHM
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
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: Sat, 11 May 2019 02:25:37 +0000
Gerrit-HasComments: No
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
Another test suggestion, but I'm not going to gate this on it if you don't want to.
http://gerrit.cloudera.org:8080/#/c/13255/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/13255/2/src/kudu/util/async_util-test.cc@168
PS2, Line 168: TimedWaitFlavor::WaitFor,
: TimedWaitFlavor::WaitUntil
Agh, in retrospect, it might be simpler to do this by passing in waiting functors that take in synchronizers and wait interval, and return a Status.
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
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: Sat, 11 May 2019 01:35:21 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduced Synchronizer::WaitUntil()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduced Synchronizer::WaitUntil()
......................................................................
Patch Set 1: Code-Review+1
(2 comments)
LGTM, just a couple test suggestions that I don't feel _that_ strongly about, but would be nice. WDYT?
http://gerrit.cloudera.org:8080/#/c/13255/1/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/13255/1/src/kudu/util/async_util-test.cc@103
PS1, Line 103: TEST_F(AsyncUtilTest, TestSynchronizerTimedWaitFor) {
nit: would be nice if we didn't duplicate this logic. Could we perhaps parameterize by some WAIT_TYPE enum?
http://gerrit.cloudera.org:8080/#/c/13255/1/src/kudu/util/async_util-test.cc@105
PS1, Line 105: thread waiter;
: SCOPED_CLEANUP({
: // Waiting on the thread gives TSAN to check that no thread safety issues
: // occurred.
: waiter.join();
nit: maybe pull this out of scope and keep the old L113 waiter.join()? That way we don't have to duplicate this at L124
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 07 May 2019 02:42:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13255/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/13255/2/src/kudu/util/async_util-test.cc@168
PS2, Line 168: TimedWaitFlavor::WaitFor,
: TimedWaitFlavor::WaitUntil
> Agh, in retrospect, it might be simpler to do this by passing in waiting fu
I don't want to make it an exercise in automated code-generation :)
I considered passing wait intervals as a parameter as well when preparing PS2, but I decided it would make the code of the methods really hard to understand. So I intentionally didn't do that. Adding functors to that would make it even worse, IMO.
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
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: Sat, 11 May 2019 02:28:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
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)
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
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/13255
to look at the new patch set (#2).
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
[util] introduce Synchronizer::WaitUntil()
Added Synchronizer::WaitUntil() method to avoid converting MonoTime
into MonoDelta when the deadline is specified as MonoTime.
Updated corresponding tests for Synchronizer as well.
Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
---
M src/kudu/master/hms_notification_log_listener.cc
M src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 76 insertions(+), 27 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/55/13255/2
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [util] introduce Synchronizer::WaitUntil()
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduce Synchronizer::WaitUntil()
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/13255/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/13255/2/src/kudu/util/async_util-test.cc@168
PS2, Line 168: TimedWaitFlavor::WaitFor,
: TimedWaitFlavor::WaitUntil
> I don't want to make it an exercise in automated code-generation :)
Yeah, two layers of parameterization for something so small seems like overkill.
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
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: Sat, 11 May 2019 05:27:22 +0000
Gerrit-HasComments: Yes
[kudu-CR] [util] introduced Synchronizer::WaitUntil()
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13255 )
Change subject: [util] introduced Synchronizer::WaitUntil()
......................................................................
Patch Set 1: Code-Review+1
Nothing to add to what Andrew already said.
--
To view, visit http://gerrit.cloudera.org:8080/13255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I00586ac1ba49494ff08abae0d452ab9286a3e56f
Gerrit-Change-Number: 13255
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Tue, 07 May 2019 03:55:48 +0000
Gerrit-HasComments: No