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: (3 comments) > (1 comment) > > Most of this looks mechanical, and it looked fine. You changed some > 1-line if's into 3-line ifs, which may be against house style. > > We don't seem to have any tests for condition-variable.h. How did > you test this? OK, I'll change the 3-line ifs back to 1-lines. After the modifications, I ran the backend test suites on my local desktop. Should I create tests for condition-variable.h? We don't seem to have unit tests yet. 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@38 PS1, Line 38: void Wait(boost::unique_lock<boost::mutex>& lock) { > I'm not yet familiar with this part of C++; why is 'inline' getting removed About the 'struct' keyword: it is something that C requires, but in C++ it can be omitted. If there were a function named 'timespec', then we would have to use the 'struct' keyword in C++ as well to resolve ambiguity. An example for that is 'stat' in POSIX, which is also a function and a struct. http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@38 PS1, Line 38: void Wait(boost::unique_lock<boost::mutex>& lock) { > "inline" is implied by the function being defined within the class body: ht Done http://gerrit.cloudera.org:8080/#/c/8428/1/be/src/util/condition-variable.h@64 PS1, Line 64: template <typename duration_type> > It looks like this is only ever used with microseconds - maybe we should na IMHO it is easier for the callers to specify a time duration than forcing them to calculate a time point in the future. At least I think this is how it is used most of the time, so I would choose this direction. We also have calls with milliseconds(thrift-server.cc, impala-server.cc), and seconds (fragment-instance-state.cc) as well. We can also narrow the scope by templates (though it is a bit ugly): template <int N> ... subsecond_duration<time_duration, N> ... Or, we can fix the callsites to always use microseconds. What do you think of it? -- 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: Thu, 02 Nov 2017 18:02:26 +0000 Gerrit-HasComments: Yes
