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 Wed, 04 Jan 2017 01:00:38 GMT
Dan Hecht has posted comments on this change.

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


Patch Set 24:

(6 comments)

Looks good. Just a few more clarification suggestions.

http://gerrit.cloudera.org:8080/#/c/5141/24//COMMIT_MSG
Commit Message:

PS24, Line 27: disabled
not implemented


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

Line 41:   /// The physical file is created when the file is first written by DiskIoMgr.
this sentence seems out of place (now that creation doesn't happen here), and is already stated
at the class level, so let's just delete it.


Line 48:   /// Called to notify TmpFileMgr that an IO error was encountered for this file.
Confusing given that TmpFileMgr is the one that calls this method (and so doesn't get notified
via this).


PS24, Line 50: ReportIOError
Maybe even rename this Blacklist() or similar, since that's the main side effect now.


PS24, Line 56: If this file does not
             :   /// reside on a known disk, e.g. it is a remote FS, disk IDs are chosen round-robin.
this is more of a detail of the disk-io-mgr code that we call through to, so maybe delete
this.


Line 58:   int DiskIdForWrite() const;
it's a bit confusing that we call both this thing and 'disk_id_' a "disk id".  How about renaming
this AssignDiskQueue() or GetDiskQueueIdx()?  (This ambiguity exists elsewhere in disk io
mgr param names but we might as well try to clear it up here).


-- 
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: 24
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