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-4835: Part 1: simplify I/O mgr mem mgmt
Date Mon, 06 Nov 2017 17:37:55 GMT
Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8414 )

Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt
......................................................................


Patch Set 4:

(3 comments)

Testing found one subtle bug.

http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr-reader-context.cc
File be/src/runtime/disk-io-mgr-reader-context.cc:

http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr-reader-context.cc@50
PS4, Line 50:   DCHECK(buffer->scan_range()->external_buffer_tag_ == ScanRange::ExternalBufferTag::NO_BUFFER)
> long line
Done


http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr.cc
File be/src/runtime/disk-io-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr.cc@462
PS4, Line 462:       // Buffers the were not allocated by DiskIoMgr don't need to be freed.
> existing typo: that
Done


http://gerrit.cloudera.org:8080/#/c/8414/4/be/src/runtime/disk-io-mgr.cc@797
PS4, Line 797:       reader->disk_states_[disk_queue->disk_id].DecrementRequestThread();
There were a subtle bug here that could cause readers to get wedged. The problem is that we
should call DecrementRequestThreadAndCheckDone() when the range is cancelled, since it's possible
that the RequestContext was cancelled in the meantime and this is the last thread that needs
to do the cleanup.

Ideally we would rethink the names of some of these functions and clarify the protocol but
I don't want to get too distracted from the main point of this work.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1
Gerrit-Change-Number: 8414
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Tianyi Wang <twang@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Mon, 06 Nov 2017 17:37:55 +0000
Gerrit-HasComments: Yes

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