Tim Armstrong has posted comments on this change. Change subject: IMPALA-3671: Add query option to limit scratch space usage ......................................................................
Patch Set 5: (20 comments) http://gerrit.cloudera.org:8080/#/c/3938/4/be/src/runtime/buffered-block-mgr-test.cc File be/src/runtime/buffered-block-mgr-test.cc: Line 1199: query_options.scratch_limit = 0; > should this case be calling InitMultipleTmpDirs() with num_dirs > 0, so tha By default it's initialized with a single temporary dir, so that should be the case. Line 1206: > could we have a test case that shows that the scratch limit calculation is I added a test case that does this in TmpFileMgrTest. http://gerrit.cloudera.org:8080/#/c/3938/4/be/src/runtime/buffered-block-mgr.cc File be/src/runtime/buffered-block-mgr.cc: PS4, Line 1326: > let's make this clearer what it's accounting. ScratchFileUsed? Line 1345: // It is possible for a device to be blacklisted after it was returned The return status was ignored here. Made it more explicit why it was ignoring it. http://gerrit.cloudera.org:8080/#/c/3938/4/be/src/runtime/buffered-block-mgr.h File be/src/runtime/buffered-block-mgr.h: Line 496: /// Returns an error only if no temporary files are usable or the scratch limit is Fixed this formatting. PS4, Line 599: 'tmp_file_gro Fixed variable name. PS4, Line 646: allocated > is the units bytes? clarify comment and consider making the name clearer: s Done http://gerrit.cloudera.org:8080/#/c/3938/4/be/src/runtime/test-env.cc File be/src/runtime/test-env.cc: PS4, Line 72: > space before Done http://gerrit.cloudera.org:8080/#/c/3938/4/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: PS4, Line 280: emplace_back > why not just push_back(tmp_file)? It's needed to initialise the unique_ptr. Line 287: Status status = file->Remove(); > drops returned status on the floor. I guess this was preserved from the old code. Can't do much with the status, so I added a warning log. http://gerrit.cloudera.org:8080/#/c/3938/4/be/src/runtime/tmp-file-mgr.h File be/src/runtime/tmp-file-mgr.h: Line 32: /// > explain the relationship between TmpFileMgr and FileGroup (or how FileGroup Done PS4, Line 54: Allocates 'write_size' bytes in this file for a new block of d > this makes it sound like we allow the allocation to cross the limit and the Done Line 63: > should this still be exposed or should we only do this through file group c Made it private and added FileGroup as a friend class. PS4, Line 116: settin > DCHECK_EQ Done Line 122: ~FileGroup(){ Renamed to match the other NewFile() method. PS4, Line 135: > in bytes? Done PS4, Line 136: > max_size makes it sound like this is the maximum file size, or something li bytes_limit works PS4, Line 137: DCHECK_LT(in > bytes_limit_? Done PS4, Line 155: s rep > bytes Done PS4, Line 190: ociated > How about calling it NewFile() (to help differentiate with GetFileAt())? 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: 5 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Bikramjeet Vig <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Tim Armstrong <[email protected]> Gerrit-HasComments: Yes
