Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19622 )
Change subject: KUDU-3413 [multi-tenancy] update server key for multi-tenancy ...................................................................... Patch Set 9: (13 comments) http://gerrit.cloudera.org:8080/#/c/19622/9//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19622/9//COMMIT_MSG@17 PS9, Line 17: after restart nit: during bootstrap ? http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@515 PS9, Line 515: key_provider_->DecryptEncryptedKey(this->tenant_key(fs::kDefaultTenantName), nit: Add TODO to describe the following work to do, I guess we will change to use the real tenant name instead of kDefaultTenantName, right? http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@526 PS9, Line 526: RETURN_NOT_OK_PREPEND(ServerKeyUpgradeToDefaultTenantKey(&encryption_key), I‘m not sure if it's supported to remove default tenant key, if it is, how to deal the stale server key? http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@648 PS9, Line 648: new nit: how about "temp" or "backup" ? "new" sounds like the latest wrriten one which is in "old" place in fact. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@655 PS9, Line 655: changed.second How about express both the source and target filename for rename operation? http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@655 PS9, Line 655: nit: remove to align. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@664 PS9, Line 664: string nit: better to add "const" http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@666 PS9, Line 666: RenameFile Also check whether rename file failed. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@682 PS9, Line 682: + nit: Keep the code style to use Substitute. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@708 PS9, Line 708: shared_lock<rw_spinlock> md_lock(metadata_rwlock_.get_lock()); If I didn't miss anythibg, the metadata is only write/rewrite sequentially when server bootstrap, why introduce this lock? http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@885 PS9, Line 885: } else if (encryption_key || encryption_key_iv || encryption_key_version) { There are some other invalid parameters like: !tenant_name && tenant_id && !encryption_key && !encryption_key_iv && !encryption_key_version tenant_name && !tenant_id && !encryption_key && !encryption_key_iv && !encryption_key_version Maybe it will never be called like this, but it will be helpful for reading if adding such CHECKs to cover all branchs. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/util/env_posix.cc File src/kudu/util/env_posix.cc: http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/util/env_posix.cc@210 PS9, Line 210: DEFINE_bool(enable_multi_tenancy, false, How about move these code to fs_manager.cc where it is actually been used? http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/util/env_posix.cc@216 PS9, Line 216: FLAGS_encrypt_data_at_rest nit: shoud be: !FLAGS_encrypt_data_at_rest ? -- 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: 9 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: Tidy Bot (241) 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: Wed, 10 May 2023 16:52:05 +0000 Gerrit-HasComments: Yes
