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 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/dir_manager.h@296 PS2, Line 296: tenant_id nit: Mark it as constant. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager-test.cc File src/kudu/fs/error_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager-test.cc@90 PS2, Line 90: s nit: maybe distinguish the names, although they are not used. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager.h File src/kudu/fs/error_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager.h@35 PS2, Line 35: The input string is the UUID a failed : // component. nit: Update the comments. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@428 PS2, Line 428: scoped_refptr There are many related changes in this patch, could you please charify the reason? Thanks. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@474 PS2, Line 474: SearchBlockManager GetBlockManager? And mark this function as constant? http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@627 PS2, Line 627: env map nit: 'env_map_' http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1156 PS2, Line 1156: AddTenant Is it possible to AddTenant() with a same tenant concurrently? It seems block_manager may be overwriten in "block_manager_map_[tenant_id] = block_manager;" http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1212 PS2, Line 1212: NotSupported Maybe return NotFound in this case? http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1226 PS2, Line 1226: push_back nit: reserve the size before the loop. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1232 PS2, Line 1232: GetEnv There are many related updates (env() to GetEnv()) in this patch, how about defining GetEnv() to return the default env_ at first in a separate patch, the patches would be smaller to review then. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1238 PS2, Line 1238: std::shared_ptr It's a bit of waste to create a shared_ptr if the tenant_id's env can be found, return the found env in this case. 'std::' can be omitted. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1379 PS2, Line 1379: Remove the space, other places are the same. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h@520 PS2, Line 520: tenant_id_ Both file block manager and log block manager have this member, why not make it as a member of super class BlockManager? http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h@539 PS2, Line 539: kudu::fs:: nit: can be omitted. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/tools/kudu-tool-test.cc@8136 PS2, Line 8136: kDefaultTenantID How about spliting the change of updating tanent_name to tanent_id to a separate patch? -- 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: 2 Gerrit-Owner: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Tue, 04 Jul 2023 06:48:39 +0000 Gerrit-HasComments: Yes
