Yuqi Du has posted comments on this change. ( http://gerrit.cloudera.org:8080/19622 )
Change subject: KUDU-3413 server key update to tenant key ...................................................................... Patch Set 2: (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: server key update to tenant key how about 'update server key to tenant key for multi-tenancy supported' This patch is one of following patches. How about adding a tag for these patches. such as: [multi-tenants] KUDU-3413 update server key to tenant key for multi-tenancy supported 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: Currently, there is no multi-tenants feature, in other words that is only one tenant exists and all tables belong to this tenant. So this patch will do some refactor to make this default tenant as a tenant of multi-tenants. http://gerrit.cloudera.org:8080/#/c/19622/2//COMMIT_MSG@14 PS2, Line 14: I want : to use this patch as the beginning of the multi-tenant : feature development. What is your complete plan at this feature? How many patches will be committed and what is the intention of every patch? I have reviewed some of changes, and I think this patch is an advanced patch. Before this patch, it seems tenants should be added first including manage tenants: CRUD a tenant. 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 md_lock_; nit: Protects 'metadata_'. and how about metadata_rwlock_ ? 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::kDefaultTenantName in memory, not add it into InstanceMetadataPB's 'repeated tenant_metadata'. If add it to InstanceMetadataPB::TenantMetadataPB it will be durable to 'instance' file and has two tenants with the same name, one is the server tenant before and the other is durable by first startup after this feature. Because fs::kDefaultTenantName is reserved for system kudu system to be compatible with the server before, we should not add another tenant named fs::kDefaultTenantName. To reduce some complex problem, I think simply dealing with it is a considerable option. What do you think this? http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@968 PS2, Line 968: std::string nit: string http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@981 PS2, Line 981: std::string nit: string http://gerrit.cloudera.org:8080/#/c/19622/2/src/kudu/fs/fs_manager.cc@986 PS2, Line 986: string how about optional<string> ? 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. Because your purpose is compatible with before, so it seems not necessary to change it for only the name changed. And the same advices for some other changes like this. -- 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: 2 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: 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: Sat, 01 Apr 2023 09:52:22 +0000 Gerrit-HasComments: Yes
