Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer ...................................................................... Patch Set 4: (20 comments) I didn't review fs_manager.cc carefully this time, I'll do it later. http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h File src/kudu/fs/block_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@211 PS4, Line 211: enum MergeReport { How about using the C++11 standard enum class ? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@212 PS4, Line 212: merge operation Merge what kind of operations? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@288 PS4, Line 288: virtual std::string tenant_id() = 0; It can be constant? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.h File src/kudu/fs/data_dirs.h: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.h@122 PS4, Line 122: kudu::fs:: Why add the namespace? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.cc File src/kudu/fs/data_dirs.cc: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.cc@252 PS4, Line 252: *dd_manager = dm; scoped_refptr has a 'swap()' member function, we can use it as well? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/dir_manager.h@296 PS4, Line 296: std::string tenant_id() { return opts_.tenant_id; } nit: mark it as constant. http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager-test.cc File src/kudu/fs/error_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager-test.cc@90 PS4, Line 90: s nit: use another name. http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h File src/kudu/fs/error_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h@35 PS4, Line 35: The input string Add some comments for the newly added parameter. http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h@149 PS4, Line 149: uuid Add some comments of the newly added parameter for the two functions below. http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc File src/kudu/fs/fs_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc@323 PS4, Line 323: NO_FATALS(InitTenantInNeed()); This line is aim to check the behavior of add duplicate tenant? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc@1082 PS4, Line 1082: scoped_refptr<DataDirManager> How about use 'auto' instead to simplify the code? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@273 PS4, Line 273: subdirectory nit: subdirectories? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@425 PS4, Line 425: from 'dd_manager_map_' I think it's not necessary to mention the internal member, the developer may be confused: is there any other function to get dd manager from any other place? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@432 PS4, Line 432: metadate nit: metadata? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@474 PS4, Line 474: SearchBlockManager Different from InitBlockManager, this function is immutable, we can declare it as constant? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@596 PS4, Line 596: with nit: keyed by ? http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@658 PS4, Line 658: the data dir manager map below nit: 'dd_manager_map_' http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@664 PS4, Line 664: the block manager map below nit: 'block_manager_map_' http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager-test.cc File src/kudu/fs/log_block_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager-test.cc@153 PS4, Line 153: std::vector<std:: nit: 'std::' can be omitted. http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager.cc@1111 PS4, Line 1111: scoped_refptr<FsErrorManager> I guess all explicit type declaration like this can be replaced by 'auto'? -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 4 Gerrit-Owner: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Wed, 19 Jul 2023 04:03:26 +0000 Gerrit-HasComments: Yes
