impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <>
Subject [Impala-ASF-CR] IMPALA-6134: Update code base to use impala::ConditionVariable
Date Mon, 06 Nov 2017 17:26:08 GMT
Tim Armstrong has posted comments on this change. ( )

Change subject: IMPALA-6134: Update code base to use impala::ConditionVariable

Patch Set 2: Code-Review+1

File be/src/runtime/
PS2, Line 220: unique_lock
This change is harmless but doesn't seem necessary.
File be/src/runtime/
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.
PS2, Line 337:     auto wait_duration = boost::posix_time::seconds(FLAGS_status_report_interval);
This is pre-existing but we should avoid shadowing variables.
File be/src/service/
PS2, Line 1800:         session_timeout_cv_.WaitFor(timeout_lock, wait_duration);
nit: could just be a one liner.
File be/src/util/condition-variable.h:
PS2, Line 67:   bool WaitFor(boost::unique_lock<boost::mutex>& lock,
Thanks, I like these new names.

To view, visit
To unsubscribe, visit

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 <>
Gerrit-Reviewer: Philip Zeyliger <>
Gerrit-Reviewer: Tim Armstrong <>
Gerrit-Reviewer: Zoltan Borok-Nagy <>
Gerrit-Comment-Date: Mon, 06 Nov 2017 17:26:08 +0000
Gerrit-HasComments: Yes

  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message