Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4180: Synchronize accesses to 
RuntimeState::reader_contexts_
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4558/2/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

Line 302:   for (DiskIoRequestContext* context: reader_contexts_) {
Nit clang-format puts a space before :, should do that or consistency.


http://gerrit.cloudera.org:8080/#/c/4558/2/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

Line 167:   /// TODO: Attach the reader context to the last row batch instead.
This TODO seems too speculative to me, unclear whether the proposed approach is 
the right one, since it's usually simpler not to attach things to row batches, 
and the current approach seems fine to me.


-- 
To view, visit http://gerrit.cloudera.org:8080/4558
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I911fda526a99514b12f88a3e9fb5952ea4fe1973
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Alex Behm <alex.b...@cloudera.com>
Gerrit-Reviewer: Michael Ho <k...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com>
Gerrit-HasComments: Yes

Reply via email to