impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
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. ( 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 <boroknagyz@cloudera.com>
Gerrit-Reviewer: Philip Zeyliger <philip@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Zoltan Borok-Nagy <boroknagyz@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Nov 2017 17:26:08 +0000
Gerrit-HasComments: Yes

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