KeDeng 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: (15 comments) Thanks for your reviews. 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. Done 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. Done 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. Done 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 The multi-tenancy feature is implemented based on the data at rest encryption feature, and the data at rest encryption can be enabled separately. In order to maximize compatibility between these two different features, different tenants in the multi-tenant scenario will use different env, and the env of different tenants will have their own encryption key initialization. To achieve this goal, the related components such as dd manager are also separated by tenants and used independently. 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? Done http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@627 PS2, Line 627: env_map > nit: 'env_map_' Done 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: > Is it possible to AddTenant() with a same tenant concurrently? It seems blo In reality, it is unlikely that the same tenant will be added simultaneously here. Subsequent requests to add new tenants will first go to the master and then be sent separately to the tserver. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1212 PS2, Line 1212: > Maybe return NotFound in this case? Done http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1226 PS2, Line 1226: > nit: reserve the size before the loop. Done http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1232 PS2, Line 1232: == fs: > There are many related updates (env() to GetEnv()) in this patch, how about Done http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1238 PS2, Line 1238: if (env) { > It's a bit of waste to create a shared_ptr if the tenant_id's env can be fo Done 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. Done 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 mak Mainly based on the existing implementation, the current file block manager and log block manager have many common members and have not been put into the 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. Done 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: > How about spliting the change of updating tanent_name to tanent_id to a sep Done -- 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, 12 Jul 2023 03:07:24 +0000 Gerrit-HasComments: Yes
