Bikramjeet Vig has posted comments on this change. Change subject: IMPALA-3671: Add query option to limit scratch space usage ......................................................................
Patch Set 2: (11 comments) http://gerrit.cloudera.org:8080/#/c/3938/2/be/src/runtime/buffered-block-mgr-test.cc File be/src/runtime/buffered-block-mgr-test.cc: Line 157: BufferedBlockMgr* mgr = CreateMgr(query_id, max_buffers, block_size, &state, query_options); > Long line Done Line 1448: TEST_F(BufferedBlockMgrTest, ScratchLimitZero) { > Nit: put this next to the NoTmpDirs test since the tests are variations of Done http://gerrit.cloudera.org:8080/#/c/3938/2/be/src/runtime/buffered-block-mgr.cc File be/src/runtime/buffered-block-mgr.cc: Line 509: status = WriteUnpinnedBlock(src); > As discussed, can you file a bug for executing this code when disable_spill will do http://gerrit.cloudera.org:8080/#/c/3938/2/be/src/runtime/buffered-block-mgr.h File be/src/runtime/buffered-block-mgr.h: Line 646: /// Amount of Scratch Space Allocated > Nit: inconsistent capitalisation compared to surrounding comments. Done http://gerrit.cloudera.org:8080/#/c/3938/2/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: Line 140: const TUniqueId& query_id, File** new_file) { > Add a DCHECK(file_group != NULL) to document/enforce the invariant. Done Line 225: file_group_->current_space_allocated_ + write_size > Does this fit on a single line now? it goes beyond 90 characters. 91 to be precise Line 282: TmpFileMgr::File* TmpFileMgr::FileGroup::GetFileAt(int index) { > Nit: I think it's a bit more readable if we define these one-line accessors Done http://gerrit.cloudera.org:8080/#/c/3938/2/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: Line 112: /// responsible for calling the Close method to delete all the files in the group > Nit: put a . on the end of comments for consistency. Done Line 115: FileGroup(TmpFileMgr* tmp_file_mgr, int64_t max_write_size = -1); > Can you add a destructor that DCHECKS that the group was closed, to make it Done Line 148: int64_t max_space_limit; > Limit makes sense to me. Just needs an _ on the end. Done http://gerrit.cloudera.org:8080/#/c/3938/2/common/thrift/generate_error_codes.py File common/thrift/generate_error_codes.py: Line 285: ("SCRATCH_LIMIT_EXCEEDED", 91, "Scratch space limit of $0 bytes exceeded.") > I'm changing my mind on this. I think we should mention spilling/spill-to-d Sounds good. Added it. -- 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: 2 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
