Amogh Margoor 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 3: (10 comments) Changes look good and code looks much cleaner now. Added few minor comments to take care of. 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 ',' http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG@10 PS3, Line 10: this patch optimizes the code structure of TmpDir to have directory : parsing and validation logic to simplify the TmpFileMgr InitCustom(). just rephrasing it: This patch simplifies TmpFileMgr::InitCustom() by refactoring parsing and validation logic. http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG@14 PS3, Line 14: validate, in this : way, the code is easier to add more custom logic for other : filesystems in future. rephrasing: "validate. It enables easier addition of custom logic for future filesystems." http://gerrit.cloudera.org:8080/#/c/17778/3//COMMIT_MSG@20 PS3, Line 20: Because the current testcases of TmpFileMgrTest already cover the : TmpDir parsing and verification, no testcases may need to be added. nit: Move this to the Test section below. 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 too many then its better to avoid friends classes. http://gerrit.cloudera.org:8080/#/c/17778/1/be/src/runtime/tmp-file-mgr-internal.h File be/src/runtime/tmp-file-mgr-internal.h: http://gerrit.cloudera.org:8080/#/c/17778/1/be/src/runtime/tmp-file-mgr-internal.h@332 PS1, Line 332: friend class TmpFileMgr; Do these friend classes need read only access to private member variables or do they even write to them ? 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 ? 1. <s3-path>:port:bytes:priority 2. <s3-path>:port 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: if (toks_option_st_idx > 1) { : // If the option starts from the index larger than 1, that means the path : // contains the port number, and the first two tokens are the path and the : // port number. : DCHECK(toks_option_st_idx == 2); : if (toks.size() < toks_option_st_idx) { : return Status( : Substitute("The scrach path should have the port number: '$0'", raw_path_)); : } : parsed_raw_path_.append(toks[0]).append(":").append(toks[1]); : } else { : parsed_raw_path_.append(toks[0]); : } I think we can move this part to corresponding sub classes 'get_raw_path()'. They know best what to do and what error to throw. http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr.cc@577 PS3, Line 577: for (int i = toks_option_st_idx; i < toks.size(); i++) { Can we create small functions for parsing tokens for bytes_limit and priority ? http://gerrit.cloudera.org:8080/#/c/17778/3/be/src/runtime/tmp-file-mgr.cc@711 PS3, Line 711: DCHECK(is_parsed_); nice! this is much cleaner now. -- 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: 3 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-Comment-Date: Wed, 29 Sep 2021 16:50:49 +0000 Gerrit-HasComments: Yes
