Zoltan Borok-Nagy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8428 )
Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable ...................................................................... Patch Set 1: (2 comments) http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h File be/src/util/condition-variable.h: http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@48 PS1, Line 48: const timespec* timeout > I feel like this should be a const&, since it has to be non-null. That woul OK, will do it. http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@65 PS1, Line 65: bool TimedWait(boost::unique_lock<boost::mutex>& lock, > This looks like it only has one callsite. There's another callsite in Block Now that I looked into the callsites more carefully, I realized that there are two use cases for TimedWait. In Impala both of them are used. One use case is when we wait until a point in time: auto deadline = calculate_deadline(); while (! condition) { if (!cv.TimedWait(lock, deadline) { // deadline reached without notification return Status::ERROR; } } The other use case is waiting in a loop, and signaling some status periodically: while (! condition) { cv.TimedWait(lock, seconds(1)); if (condition) break; else SendReport(); } Both API can be expressed in terms of the other, however it is a bit awkward (expressing deadline in terms of duration is more awkward, so if we have to choose, I think we should go with supporting deadlines). There are examples for both in the code base: FragmentInstanceState::ReportProfileThread: wait duration in terms of wait until deadline. Promise::Get: wait until deadline in terms of wait duration. Now I am on the side of support both use case with the two APIs, like std::condition_variable has wait_for and wait_until member functions. Maybe we can use these names also to be more explicit. What do you think? -- To view, visit http://gerrit.cloudera.org:8080/8428 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3085c6dcb42350b61244df6e7f091a1e7db356c9 Gerrit-Change-Number: 8428 Gerrit-PatchSet: 1 Gerrit-Owner: Zoltan Borok-Nagy <[email protected]> Gerrit-Reviewer: Philip Zeyliger <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-Reviewer: Zoltan Borok-Nagy <[email protected]> Gerrit-Comment-Date: Fri, 03 Nov 2017 10:05:26 +0000 Gerrit-HasComments: Yes
