Lars Volker has posted comments on this change. Change subject: IMPALA-3202: refactor scratch file management into TmpFileMgr ......................................................................
Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/4898/1/be/src/runtime/buffered-block-mgr-test.cc File be/src/runtime/buffered-block-mgr-test.cc: PS1, Line 308: EXPECT_TRUE ASSERT_TRUE? Otherwise we will hit a NPE in DeleteBlocks(). http://gerrit.cloudera.org:8080/#/c/4898/1/be/src/runtime/buffered-block-mgr.cc File be/src/runtime/buffered-block-mgr.cc: Line 246: ->Init(state->io_mgr(), tmp_file_mgr, profile, parent, mem_limit, scratch_limit); This line break looks odd. If it was done by clang-format I'd keep it, but otherwise maybe break after Init( . http://gerrit.cloudera.org:8080/#/c/4898/1/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: Line 282: DCHECK_EQ(tmp_files_.size(), 0); nit: DCHECK(tmp_files_.empty()); PS1, Line 290: by ignoring the return status of : // NewFile(). we don't seem to actually ignore it, the comment looks wrong. PS1, Line 296: spilling "No scratch directories..."? We seem to use 'tmp', 'scratch', and 'spilling' interchangeably, maybe reduce to one or two of them? Line 335: scratch_space_bytes_used_counter_->Add(num_bytes); can we handle current_bytes_allocated_ here, too? That's also the only direct write access I could find from File to a field of FileGroup, so maybe we can unfriend them, too. Line 336: return Status::OK(); nit: this could just be "return status". Line 351: err_status.MergeStatus(errs[i]); nit: single line http://gerrit.cloudera.org:8080/#/c/4898/1/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: PS1, Line 133: nit: double space PS1, Line 136: query_id does this actually have to be the query_id or could we rename it to something like filegroup_id? -- To view, visit http://gerrit.cloudera.org:8080/4898 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0c56c195f3f28d520034f8c384494e566635fc62 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Tim Armstrong <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-HasComments: Yes
