Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/17778 )
Change subject: IMPALA-10862 Optimization of the code structure of TmpDir ...................................................................... Patch Set 4: (9 comments) http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG@9 PS3, Line 9: . > '.' instead of ',' Done http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG@10 PS3, Line 10: : This patch simplifies TmpFileMgr::InitCustom() by refactoring parsing > just rephrasing it: This patch simplifies TmpFileMgr::InitCustom() by refac Done http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG@14 PS3, Line 14: S3 to inherit TmpDir : to implement their own logic to parse and validate. It enables easier : addition of custom log > rephrasing: "validate. It enables easier addition of custom logic for futur Done http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG@20 PS3, Line 20: doesn't change. : > nit: Move this to the Test section below. Done http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr-internal.h File be/src/runtime/tmp-file-mgr-internal.h: http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr-internal.h@332 PS3, Line 332: friend class TmpFileMgr; > nit: which private variables are exposed to these friend classes ? If not t remove some, leave the TmpDir subclasses as friend classes. http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr-test.cc File be/src/runtime/tmp-file-mgr-test.cc: http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr-test.cc@1109 PS3, Line 1109: EXPECT_EQ("s3a://fake_host/for-parsing-test-only/impala-scratch", dirs15->path()); > Do we have any tests for negative S3 paths ? Added. http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr.cc File be/src/runtime/tmp-file-mgr.cc: http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr.cc@551 PS3, Line 551: // ["path", "bytes_limit", "priority"]. : parsed_raw_path.append(toks[0]); : *path = parsed_raw_path; : *offset = 1; : return Status::OK(); : } : : Status TmpDir::ParsePath(const vector<string>& toks, int* offset) { : string parsed_raw_path; : RETURN_IF_ERROR(GetPathFromToks(toks, &parsed_raw_path, offset)); : parsed_raw_path = trim_right_copy_if(parsed_raw_path, is_any_of("/")); : : / > I think we can move this part to corresponding sub classes 'get_raw_path()' Added a new function GetPathFromToks(). http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr.cc@577 PS3, Line 577: DCHECK_GE(index, 0); > Can we create small functions for parsing tokens for bytes_limit and priori Done http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr.cc@711 PS3, Line 711: << "Error was: " << status.msg().msg(); > nice! this is much cleaner now. Thank you for the suggestion! -- To view, visit http://gerrit.cloudera.org:8080/17778 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I52971238d5841a1cdfee06b38750f9dc99a6a2be Gerrit-Change-Number: 17778 Gerrit-PatchSet: 4 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Amogh Margoor <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Thu, 30 Sep 2021 04:30:48 +0000 Gerrit-HasComments: Yes
