impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3202,IMPALA-2079: rework scratch file I/O
Date Mon, 19 Dec 2016 23:58:44 GMT
Dan Hecht has posted comments on this change.

Change subject: IMPALA-3202,IMPALA-2079: rework scratch file I/O
......................................................................


Patch Set 19:

(6 comments)

Just replies to previous comments or code changed for previous comments.

I think i'll probably continue to review the rest after you address the remaining outstanding
comments so we don't have too many in flight.

http://gerrit.cloudera.org:8080/#/c/5141/19/be/src/runtime/disk-io-mgr.h
File be/src/runtime/disk-io-mgr.h:

Line 868:   /// not associated with a particular local disk or remote queue). Use to implement
weird line break


http://gerrit.cloudera.org:8080/#/c/5141/16/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:

Line 125:     /// Synchronously read the data referenced by 'handle' from the temporary file
into
> Yeah I had trouble with better names. Cancel seems okish. Maybe Discard or 
Yeah, maybe DestroyWriteHandle()?


PS16, Line 243: 
> Yeah. "Scratch" is the user-facing name so I could just rename all the inte
I'm fine with renaming the classes (or not). Mostly just concerned about being consistent
in terminology when we can (or at least explicitly saying up front that they are synonymous).


http://gerrit.cloudera.org:8080/#/c/5141/19/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:

PS19, Line 249: and
then?


Line 250:   ///
i think it would be good to explain the lifecycle of this thing here. you can piece it together
from the method comments of TmpFileMgr, but a quick summary here would help give the bigger
picture of how this is used.


Line 330:     /// before the FileGroup (which has query lifetime).
this is making assumptions about how FileGroup is used, and this will change with BufferPool,
right? I'm not saying we have to clean it all up now, but this seems like it needs cleanup
(maybe after we switch to bufferpool).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c9c587df006d2f09d72dd636adafbd295fcdc17
Gerrit-PatchSet: 19
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message