Tim Armstrong 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 2: Code-Review+1 (5 comments) http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/data-stream-recvr.cc File be/src/runtime/data-stream-recvr.cc: http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/data-stream-recvr.cc@220 PS2, Line 220: unique_lock This change is harmless but doesn't seem necessary. http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/fragment-instance-state.cc File be/src/runtime/fragment-instance-state.cc: http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/fragment-instance-state.cc@332 PS2, Line 332: auto wait_duration = boost::posix_time::seconds(report_fragment_offset); The use of "auto" here is probably marginal as far as our coding idioms go. I think this case is fine, but we're generally biased towards spelling out the type when it is reasonably concise and meaningful. I.e. we'd use auto to save typing and reading something like "vector<int>::iterator", but usually avoid it when it's just a plain class name. http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/runtime/fragment-instance-state.cc@337 PS2, Line 337: auto wait_duration = boost::posix_time::seconds(FLAGS_status_report_interval); This is pre-existing but we should avoid shadowing variables. http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/service/impala-server.cc File be/src/service/impala-server.cc: http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/service/impala-server.cc@1800 PS2, Line 1800: session_timeout_cv_.WaitFor(timeout_lock, wait_duration); nit: could just be a one liner. http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/util/condition-variable.h File be/src/util/condition-variable.h: http://gerrit.cloudera.org:8080/#/c/8428/2/be/src/util/condition-variable.h@67 PS2, Line 67: bool WaitFor(boost::unique_lock<boost::mutex>& lock, Thanks, I like these new names. -- 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: 2 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: Mon, 06 Nov 2017 17:26:08 +0000 Gerrit-HasComments: Yes
