You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2018/06/21 19:40:44 UTC
[kudu-CR] Synchronizer::WaitFor thread-safety
Hello Adar Dembo, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/10783
to review the following change.
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 151 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/1
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Reviewed-on: http://gerrit.cloudera.org:8080/10783
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 170 insertions(+), 31 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 2:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@38
PS2, Line 38: public:
> Not needed?
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46
PS2, Line 46: auto waiter = thread([sync] {
> Aren't these captures by copy? Isn't that disallowed? How does this work?
Correct. I removed the DISALLOW_COPY_AND_ASSIGN call on Synchronizer since it's no longer necessary for correctness. This test uses the API somewhat unusually in that it waits on the synchronizer from the background thread and completes the callback on the original thread, but I don't think that's really a problem. The more conventional orientation is tested in TestSynchronizerTimedWait.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@47
PS2, Line 47: auto s = sync.Wait();
> You're not using 's'; could you just omit the LHS? Or wrap the call in igno
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@96
PS2, Line 96: waiter = thread([cb] {
> Capture by ref here and below?
That would be a use after free in the second case, since the callback would be cleaned up before the sleep is complete. In the first case I think it's more typical for the callback to be moved into the background thread.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@100
PS2, Line 100: CHECK_OK
> ASSERT_OK?
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@111
PS2, Line 111: CHECK
> ASSERT
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h
File src/kudu/util/async_util.h:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@33
PS2, Line 33: // Simple class which can be used to make async methods synchronous.
> Can you update this to describe how the lifecycle of the callbacks is decou
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44
PS2, Line 44: void StatusCB(const Status& status) {
> No longer used? If it is used (i.e. for non-callback invocations on the syn
It's used in a test, and it's part of the public API and seems reasonably useful so I didn't want to remove it. Having them share an implementation would be pretty awkward:
void StatusCB(const Status& status) {
Data::Callback(std::weak_ptr(data_), status);
}
But if that seems better to you then I can make the change.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@71
PS2, Line 71: DCHECK_EQ(data_.use_count(), 1);
> Are you sure that weak refs count towards use_count()? I don't think they d
Yeah you're right. I changed this initially because it smelled like a Java-style memory leak where a field is failed to be reset to null, but in practice it won't matter as you point out.
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 19:27:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 5:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41
PS2, Line 41: class AsyncUtilTest : public KuduTest {
> Looks good, but why not add it to the test fixture's constructor/destructor
Done
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46
PS2, Line 46: }
> Oh, I see. I missed that in the first pass. Could you add DISALLOW_COPY_AND
Good idea, done.
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:32:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10783
to look at the new patch set (#5).
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 172 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/5
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41
PS2, Line 41:
> If the hypothetical failure mode for this test is a deadlock or other test
Done
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 19:28:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:55:12 +0000
Gerrit-HasComments: No
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10783
to look at the new patch set (#8).
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 170 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/8
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 2:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@38
PS2, Line 38: public:
Not needed?
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41
PS2, Line 41: // Test completing the synchronizer through each of the APIs it exposes.
If the hypothetical failure mode for this test is a deadlock or other test hang, consider calling alarm(30) or some such in the beginning of the test (and adding a ScopedCleanup for alarm(0) right after). You could potentially build it into the test fixture too.
Below too.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46
PS2, Line 46: auto waiter = thread([sync] {
Aren't these captures by copy? Isn't that disallowed? How does this work?
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@47
PS2, Line 47: auto s = sync.Wait();
You're not using 's'; could you just omit the LHS? Or wrap the call in ignore_result()?
Below too.
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@96
PS2, Line 96: waiter = thread([cb] {
Capture by ref here and below?
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@100
PS2, Line 100: CHECK_OK
ASSERT_OK?
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@111
PS2, Line 111: CHECK
ASSERT
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h
File src/kudu/util/async_util.h:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@33
PS2, Line 33: // Simple class which can be used to make async methods synchronous.
Can you update this to describe how the lifecycle of the callbacks is decoupled from that of the synchronizer itself, and that they may outlive the synchronizer if needed?
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44
PS2, Line 44: void StatusCB(const Status& status) {
No longer used? If it is used (i.e. for non-callback invocations on the synchronizer), it should at least share code with Callback().
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@71
PS2, Line 71: DCHECK_EQ(data_.use_count(), 1);
Are you sure that weak refs count towards use_count()? I don't think they do, which significantly weakens the protection offered by this DCHECK.
Is it really that important to reset data_->status? Aren't we guaranteed to overwrite it any time the latch counts down? If we omitted the reset of data_->status, there'd be no race, right?
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 21 Jun 2018 20:39:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10783
to look at the new patch set (#3).
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 178 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/3
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10783
to look at the new patch set (#6).
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 171 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/6
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h
File src/kudu/util/async_util.h:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44
PS2, Line 44: class Synchronizer {
> I would prefer that, yes. Only one place where CountDown() is used, and if
Done
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:33:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10783
to look at the new patch set (#4).
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 180 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/4
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 6: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc@48
PS6, Line 48: alar
alarm
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 6
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:48:15 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10783
to look at the new patch set (#2).
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 155 insertions(+), 30 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/2
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 8: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 21:31:58 +0000
Gerrit-HasComments: No
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/6/src/kudu/util/async_util-test.cc@48
PS6, Line 48: alar
> alarm
Done
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:50:28 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/10783 )
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Patch Set 4:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc
File src/kudu/util/async_util-test.cc:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@41
PS2, Line 41: class AsyncUtilTest : public KuduTest {
> Done
Looks good, but why not add it to the test fixture's constructor/destructor? Then you wouldn't have to repeat yourself?
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util-test.cc@46
PS2, Line 46: // Set up an alarm to fail the test in case of deadlock.
> Correct. I removed the DISALLOW_COPY_AND_ASSIGN call on Synchronizer since
Oh, I see. I missed that in the first pass. Could you add DISALLOW_COPY_AND_ASSIGN to Synchronizer::Data, since that really shouldn't be copied (and probably can't?).
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h
File src/kudu/util/async_util.h:
http://gerrit.cloudera.org:8080/#/c/10783/2/src/kudu/util/async_util.h@44
PS2, Line 44: class Synchronizer {
> It's used in a test, and it's part of the public API and seems reasonably u
I would prefer that, yes. Only one place where CountDown() is used, and if the actual data ever changed to be more than just the status, only one place to make that change.
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Fri, 22 Jun 2018 20:08:00 +0000
Gerrit-HasComments: Yes
[kudu-CR] Synchronizer::WaitFor thread-safety
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/10783
to look at the new patch set (#7).
Change subject: Synchronizer::WaitFor thread-safety
......................................................................
Synchronizer::WaitFor thread-safety
WaitFor's implementation previously was not thread safe in the case that
the waiter deallocated the Synchronizer after receiving a timeout.
Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
---
M src/kudu/util/CMakeLists.txt
A src/kudu/util/async_util-test.cc
M src/kudu/util/async_util.h
3 files changed, 171 insertions(+), 31 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/83/10783/7
--
To view, visit http://gerrit.cloudera.org:8080/10783
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9565a50839ffd23b5bac6986a6fdee41ac21aa3a
Gerrit-Change-Number: 10783
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>