KeDeng 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 10: (13 comments) Thanks for your reviews! 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: during bootst > nit: during bootstrap ? Done 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: // > nit: Add TODO to describe the following work to do, I guess we will change Done http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@526 PS9, Line 526: // The tenant key should not exist for this case. > I‘m not sure if it's supported to remove default tenant key, if it is, how If you simply remove the default tenant key, you can add an interface implementation. But in a multi tenant scenario, I think this operation should be prohibited. The upgraded server key is currently reserved for future development with rollback and compatibility features. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@648 PS9, Line 648: > nit: how about "temp" or "backup" ? "new" sounds like the latest wrriten on Done http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@655 PS9, Line 655: o& changed : chang > nit: remove to align. Done http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@655 PS9, Line 655: > How about express both the source and target filename for rename operation? Done http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@664 PS9, Line 664: ; > nit: better to add "const" Done http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@666 PS9, Line 666: st auto& r > Also check whether rename file failed. Done 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. Done http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@708 PS9, Line 708: } > If I didn't miss anythibg, the metadata is only write/rewrite sequentially Yes, I add this lock for update metadata. Currently, only write/rewrite sequentially when server bootstrap, but metadata may be accessed concurrently during multi tenant implementations in the future. Therefore, I have added locks to prevent future omissions. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/fs/fs_manager.cc@885 PS9, Line 885: tenant_metadata->set_tenant_key(*encryption_key); > There are some other invalid parameters like: Thanks for your advice. And I have changed the branches. 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_validator(encryption_key_length, > How about move these code to fs_manager.cc where it is actually been used? Well, we need to use the flag for KuduTest::GetEncryptionKey() in test_util.cc. So we have no better choice. http://gerrit.cloudera.org:8080/#/c/19622/9/src/kudu/util/env_posix.cc@216 PS9, Line 216: ced); > nit: shoud be: 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: 10 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: Thu, 11 May 2023 03:36:42 +0000 Gerrit-HasComments: Yes
