[Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable

["Zoltan Borok-Nagy (Code Review)" <ge...@cloudera.org>]

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 <bo...@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <ph...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <bo...@cloudera.com>
Gerrit-Comment-Date: Thu, 02 Nov 2017 18:02:26 +0000
Gerrit-HasComments: Yes