Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-3671: Add query option to limit scratch space usage ......................................................................
Patch Set 1: (21 comments) http://gerrit.cloudera.org:8080/#/c/3938/1/be/src/runtime/buffered-block-mgr.cc File be/src/runtime/buffered-block-mgr.cc: Line 216: disable_spill_(state->query_ctx().disable_spilling || block_write_threshold_ == 0), > There's a subtle bug here if you have scratch_limit = 0, because the block Done Line 245: block_mgr->reset( > Don't need newline after reset( Done Line 827: return status; > Add a brief comment. Something like: Done http://gerrit.cloudera.org:8080/#/c/3938/1/be/src/runtime/buffered-block-mgr.h File be/src/runtime/buffered-block-mgr.h: PS1, Line 584: tmp_file_grp > maybe just tmp_file_group? It will be hard to remember the abbreviation. Done http://gerrit.cloudera.org:8080/#/c/3938/1/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: Line 224: && fileGroup_->current_write_size_ + write_size > && should go on previous line, and I think this condition fits on one line. Done Line 226: return Status(TErrorCode::SCRATCH_LIMIT_EXCEEDED); > Let's include the limit value in the error message, will make it easier for Done PS1, Line 271: AddFile > AddNewFile()? Done Line 287: file.Remove(); > I think we need to call delete on these files: in the tmp-file-mgr.h it say there is not need to delete explicitly as tmp_files.clear() destroys all objects in the container PS1, Line 292: Size > Maybe NumFiles()? Size is a little ambiguous: could mean the total bytes. Done Line 296: int64_t TmpFileMgr::FileGroup::SetSpaceAllocationLimit(int64_t max_size){ > missing space before { Done http://gerrit.cloudera.org:8080/#/c/3938/1/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: Line 88: /// The FileGroup this belongs to. Can be null if it does not belong to any group. > I feel like we should make file_group_ a required field to simplify things, Done PS1, Line 89: fileGroup_ > file_group_ (not camel case). Done Line 110: /// Represents a group of files. The total allocated bytes of the group can be > The wrapping of some of these comments is a little weird - we normally wrap Done Line 124: File* GetFileAt(int index); > I think these methods need one-line comments. Done PS1, Line 126: Remove > Remove() Done Line 129: int64_t SetSpaceAllocationLimit(int64_t max_size); > If we're not using the return value anywhere, I'd say just make this void. Done PS1, Line 131: bool > why bool? Done Line 134: > re,move extra space. Done PS1, Line 141: current_write_size_ > I think the comment would actually be a better variable name: current_space Done Line 144: int64_t max_write_size_; > max_space_allocated_? "max space allocated" might be a bit misleading. How does "max_space_limit_" sound? http://gerrit.cloudera.org:8080/#/c/3938/1/common/thrift/ImpalaService.thrift File common/thrift/ImpalaService.thrift: Line 240: // otherwise specified either as: > More concise: Done -- To view, visit http://gerrit.cloudera.org:8080/3938 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie3e9039ad1c0df978ab71b956af33fd997714e73 Gerrit-PatchSet: 1 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
