impala-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Bikramjeet Vig (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-3671: Add query option to limit scratch space usage
Date Wed, 17 Aug 2016 18:04:24 GMT
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 <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Bikramjeet Vig <bikramjeet.vig@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message