KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/19622 )
Change subject: KUDU-3413 [multi-tenancy] add tenant info in metadata for multi-tenancy ...................................................................... Patch Set 12: (10 comments) Thank you for your suggestion. I have removed the rename and upgrade related parts, and the upgrade process will be integrated into the CLI tool in the next patch. I would greatly appreciate it if you could help me review the remaining code again. http://gerrit.cloudera.org:8080/#/c/19622/11//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19622/11//COMMIT_MSG@16 PS11, Line 16: ey are present in the metadata, we need to pay attention to : the fact that tenant key will have a higher priority. That is, we will : consider it as enabling the multi-tenancy feature and ignore the processing : of server key. > What happens when a tablet server of prior versions or a tablet server of n In the previous design, if '--enable_multi_tenancy' was set to false, we will not modify any metadata. However, I have removed this part of the upgrade process and will use CLI tools to control the upgrade in the future. This flag only indicates whether to enable multi tenant features and will not upgrade metadata. http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs.proto File src/kudu/fs/fs.proto: http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs.proto@47 PS11, Line 47: the base of the server_key, but the server_key is not : // always > Please rephrase this. The compatibility isn't something that affects 'us' Done http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: PS11: > Why to have all this code in the runtime of a tablet server when it's is su Done http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/fs_manager.cc@513 PS11, Line 513: return Status::IllegalState( > Why to crash here if FLAGS_enable_multi_tenancy is set 'false' even in RELE Done http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/fs/key_provider.h File src/kudu/fs/key_provider.h: PS11: > Could you please separate this and other simple renaming changes into its o Done http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/external_mini_cluster.cc File src/kudu/mini-cluster/external_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/external_mini_cluster.cc@1374 PS11, Line 1374: tance");; > Can a valid 'tenant_name' be empty? Yes, the 'tenant_name' will work with the instance. If no tenants info exist in instance file, this param will do nothing. http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/external_mini_cluster.cc@1380 PS11, Line 1380: const InstanceMetadataPB::TenantMetadataPB* tenant = nullptr; : for (const auto& tdata : instance.tenants()) { : if (tdata.tenant_name() == tenant_name) { : tenant = &tdata; : break; : } : } : if (tenant && !tenant->tenant_key().empty()) { : RETURN_NOT_OK(key_provider_->DecryptEncryptionKey(tenant->tenant_key(), : tenant->tenant_key_iv(), : tenant->tenant_key_version(), : &decrypted_key)); : } : } else if (!instance.server_key().empty()) { : RETURN_NOT_OK(key_provider_->DecryptEncryptionKey(instance.server_key(), : instance.server_key_iv(), : instance.server_key_version(), : &decrypted_key)); : } : > I'm not sure this reflects the priority of tenant keys described in the com I have fixed it. http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/internal_mini_cluster.cc File src/kudu/mini-cluster/internal_mini_cluster.cc: http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/mini-cluster/internal_mini_cluster.cc@228 PS11, Line 228: &options->server_key_info.server_key, : &options->server_key_info.server_key_iv, : &options->server_key_info.server_key_version); : > These two arguments aren't used in this code. Maybe, it makes sense to all Done http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/util/env_posix.cc@216 PS11, Line 216: TAG_FLAG(enable_multi_tenancy > Shouldn't this flag be marked as 'experimental' at this point? Done http://gerrit.cloudera.org:8080/#/c/19622/11/src/kudu/util/env_posix.cc@221 PS11, Line 221: LOG(ERROR) << strings::Substitute( : "The --enable_mul > It's not what the condition for the if() actually checks for (otherwise, it Done -- 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: 12 Gerrit-Owner: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com> Gerrit-Reviewer: Yifan Zhang <chinazhangyi...@163.com> Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Reviewer: Yuqi Du <shenxingwuy...@gmail.com> Gerrit-Comment-Date: Thu, 25 May 2023 07:39:38 +0000 Gerrit-HasComments: Yes
