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-3202,IMPALA-2079: rework scratch file I/O
Date Tue, 20 Dec 2016 15:11:34 GMT
Tim Armstrong has posted comments on this change.

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


Patch Set 17:

(22 comments)

http://gerrit.cloudera.org:8080/#/c/5141/16/be/src/runtime/buffered-block-mgr.cc
File be/src/runtime/buffered-block-mgr.cc:

Line 651:       status = RepinInMemBlock(block);
> why do we need to do this since the block wasn't in memory in this case?
The case when the block is unpinned but the buffer is still in memory with the data - so we
don't need to do any I/O but may need to destroy the write handle and decrypt the data.


Line 669:     if (!status.ok()) goto error;
> if the read failed, why don't we have to still CloseWrite()?
It gets cleaned up when the block is deleted in that case (same if we hit an error in TransferBuffer(),
etc).


Line 687:     WaitForWrite(lock, block);
I realised I should also check for cancellation here.


Line 691:     if (block->valid_data_len() == 0) {
> when is the 0 len case used?
I ended up removing the special case.


Line 751:   // Workaround to avoid adding a special code path for the 0-byte edge case.
> what is that special case? can we not issue writes of zero length to diskio
BufferPool won't need this since it doesn't have the valid_data_len_ stuff and will just flush
the whole buffer to disk.

Zero-length writes should work but seem like an easy thing to break. I removed the special
case.


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

PS16, Line 403: evicted from
              :   /// memory
> I'm confused by the name "InMemBlock" given this says it handles blocks tha
Yeah this was inaccurate. Rewrote to make it clear that it's a block that was written to disk
or has an in-flight write.


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
Done


http://gerrit.cloudera.org:8080/#/c/5141/16/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

Line 49
> also not sure if you meant to include this?
I ran into the default max_errors value problem and saw this code was stale. The flag doesn't
exist and we have a better way to set default query options. Thought it was best to just delete
it.


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

Line 28: /// can be allocated and files removed using AllocateSpace() and Remove().
> let's clarify that File should only used internally TmpFileMgr
Done


PS16, Line 41: offset
> .. to the location of the newly allocated file space?
Done


PS16, Line 55:  it is a remote FS, disk IDs are chosen round-robin.
> does this preserve existing behavior for temporary files? for data files, w
Yeah this preserved the old behaviour, which was copy-pasted from DiskIoMgr. I switched to
using the DiskIoMgr implementation because there wasn't any reason to duplicate the logic.


PS16, Line 65: created
> creates?
Done


Line 83:   const int disk_id_;
> do we need to keep DeviceId around, or could we just use disk_ids?
We could, but then we'd have to maintain a sparse map of 'disk_id' -> 'device_id'. And
it would break all of the backend tests that rely on being able to have multiple temporary
directories per physical disk.


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

PS16, Line 150: 
> sometimes we pass as const&, other times by value. Is there a reason for th
Nope. Changed it to just pass by value. This is the only case I could find.

I think this was probably a result of over-zealous application of the "always pass classes
by const ref" rule.


PS16, Line 209: 
> ActiveTmpDevices
Done


PS16, Line 394: 
              : 
> given that we now accumulate errors, why say this?
It looks like there are some cases where the error logging mechanism ignores the merged status
messages. I filed a JIRA about that: IMPALA-4697. Added a TODO here to fix that.


PS16, Line 431: 
              :     bytes_rea
> that's always true. is this trying to say that the Read() is synchronous an
Yes - clarified it.


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

Line 76: 
> do external classes still care about DeviceId's, now that File's aren't exp
It's only useful for TmpFileMgrTest now - added a comment to explain.


Line 125:     /// Synchronously read the data referenced by 'handle' from the temporary file
into
> Yeah, maybe DestroyWriteHandle()?
Done


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?
Done


Line 250:   ///
> i think it would be good to explain the lifecycle of this thing here. you c
Done


Line 330:     /// before the FileGroup (which has query lifetime).
> this is making assumptions about how FileGroup is used, and this will chang
I think it's an orthogonal issue - which is that all of error logging is done via RuntimeState.
I'm not sure if there's a short-term plan to move that to QueryState. Anyway, we don't need
this here since we now return all the errors in a merged status, so I removed it and simplified
everything accordingly.


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