KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/19622 )
Change subject: [multi-tenancy] KUDU-3413 update server key for multi-tenancy ...................................................................... Patch Set 3: (9 comments) http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG@7 PS2, Line 7: ancy] KUDU-3413 update server k > how about 'update server key to tenant key for multi-tenancy supported' Done http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG@10 PS2, Line 10: we have to solve the : issue of version upgrade between the two features > Some description may add, such as: Done http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG@14 PS2, Line 14: h the : tenant key, which belongs to the default tenant. I want : to use this patch as > What is your complete plan at this feature? As I mentioned in the development plan in https://docs.google.com/document/d/1hcZC9qsMB4OxlJc-yYQk3JISXTCiofshG5K31_h3zas/edit?usp=sharing?there will be about 6 jobs and about 10 patches for review purposes. Before adding tenant management, it is necessary to handle the upgrade issue between the new and old versions, as the new version requires persistent metadata, which requires relevant upgrade support from the old version. http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.h@530 PS2, Line 530: // Lock protecting the 'metadata_' below. : mutable percpu_rwlock metadata_ > nit: Protects 'metadata_'. Done http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@915 PS2, Line 915: InstanceMetadataPB::TenantMetadataPB* tenant_metadata = metadata->add_tenants(); : tenant_metadata->set_tenant_key_version(key_version); : tenant_metadata->set_tenant_name(fs::kDefaultTenantName); : tenant_metadata->set_tenant_key(key); : tenant_metadata->set_tenant_key_iv(key_iv); > Whether simply convert the default tenant(the server tenant) as fs::kDefaul That's a good idea. But in addition to being inconvenient to understand, there are also other issues. If we reserve the server key for the default tenant, subsequent tenant management implementations require special treatment for this tenant, which will make the code implementation complex and ugly. The default tenant will not be allowed to be created by the user, creating a tenant with the same name will also be prohibited, so this issue can be resolved in the process. http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@968 PS2, Line 968: string& ten > nit: string Done http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@981 PS2, Line 981: string& ten > nit: string Done http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@986 PS2, Line 986: string > how about optional<string> ? This replacement does not seem to simplify the code, so it is still necessary to determine whether there is a value when using it? http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/key_provider.h File src/kudu/fs/key_provider.h: http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/key_provider.h@33 PS2, Line 33: DecryptTenantKey > IMO. Thanks for your reviews. The purpose of the upgrade is to replace the use of server keys in previous versions. There are two specific actions to implement this goal: One is the compatibility of metadata used in the existing version, which increases space for subsequent tenant management while retaining the existing key information; In addition, the adaptation of relevant usage methods. If the original name is retained, it may lead to difficulties in understanding in subsequent development and reading. Such as an API similar to XXServerKey appears to correspond to parameters of XXTenant, which may not look good. -- To view, visit http://gerrit.cloudera.org:8080/19622 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9e450d73940eb1dbaac6f905a46d6ccd084f15cf Gerrit-Change-Number: 19622 Gerrit-PatchSet: 3 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Mon, 03 Apr 2023 06:55:17 +0000 Gerrit-HasComments: Yes
