Bikramjeet Vig has posted comments on this change. ( http://gerrit.cloudera.org:8080/13986 )
Change subject: IMPALA-8376: directory limits for scratch usage ...................................................................... Patch Set 3: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/13986/3/be/src/runtime/tmp-file-mgr-test.cc File be/src/runtime/tmp-file-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/13986/3/be/src/runtime/tmp-file-mgr-test.cc@785 PS3, Line 785: file_group_1 nit: file_group_2? http://gerrit.cloudera.org:8080/#/c/13986/3/be/src/runtime/tmp-file-mgr-test.cc@834 PS3, Line 834: auto& empty_paths = GetTmpDirs(CreateTmpFileMgr(",")); so this means I can specify the same dir twice, potentially with different limits? http://gerrit.cloudera.org:8080/#/c/13986/3/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/13986/3/be/src/runtime/tmp-file-mgr.cc@131 PS3, Line 131: Interpret -1 I am wondering if we should also default to ignoring the dir in case of limit parsing failure, just like we do for path parsing failure. Like if the user wanted to put a limit on a dir, then they might prefer it failing to create that dir rather than having an infinite limit on it. Dont have a strong preference either way so I'll leave it upto you http://gerrit.cloudera.org:8080/#/c/13986/3/be/src/runtime/tmp-file-mgr.cc@178 PS3, Line 178: TmpDir( nit: dont need to call constructor explicitly for emplace_back -- To view, visit http://gerrit.cloudera.org:8080/13986 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I696146a65dbb97f1ba200ae472358ae2db6eb441 Gerrit-Change-Number: 13986 Gerrit-PatchSet: 3 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Abhishek Rawat <ara...@cloudera.com> Gerrit-Reviewer: Bikramjeet Vig <bikramjeet....@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 02 Aug 2019 22:55:13 +0000 Gerrit-HasComments: Yes
