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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Fri, 22 Jun 2018 20:08:00 +0000 Gerrit-HasComments: Yes
