impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Dan Hecht (Code Review)" <>
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:


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.
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
File be/src/runtime/tmp-file-mgr.h:

Line 125:     /// Synchronously read the data referenced by 'handle' from the temporary file
> 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).
File be/src/runtime/tmp-file-mgr.h:

PS19, Line 249: and

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
To unsubscribe, visit

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

View raw message