Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20343 )
Change subject: [multi-tenancy] add tenancy metadata on master ...................................................................... Patch Set 5: (7 comments) I took a quick look at this patch, and posted a few comments without digging deeper into details. http://gerrit.cloudera.org:8080/#/c/20343/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20343/5//COMMIT_MSG@9 PS5, Line 9: implementing multi-tenancy I'd suggest to at least use create a item in the Apache JIRA and use its ID in commits that are part of the implementation of a big task like this. In the umbrella JIRA for the task, you could use sub-tasks to track the works on some particular part of the project. Having JIRA ids referenced in the commit messages makes it much easier to track and review if talking about checking against the design, etc. Thanks! http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@279 PS5, Line 279: enum State { : UNKNOWN = 0; : PREPARING = 1; : RUNNING = 2; : ALTERING = 3; : REMOVED = 4; : } Please add a comment to explain what each 'State' is about. http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@288 PS5, Line 288: required Here and below: please don't use 'required' for newly introduced PB fields -- it will bring us more headache if/when transitioning to protobuf3. http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@288 PS5, Line 288: tenant_name Here and below: why to add 'tenant_' prefix for these fields? Also, the current naming is inconsistent: the first 3 fields have the 'tenant_' prefix, but the rest of them -- don't. http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@294 PS5, Line 294: tenant_key_version If that's a version, why not to use a numeric type instead of a string instead? http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@300 PS5, Line 300: create nit: creation http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/tenant_manager.h File src/kudu/master/tenant_manager.h: http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/tenant_manager.h@93 PS5, Line 93: the below mutable fields There is anything below, so maybe drop this lock at all? -- To view, visit http://gerrit.cloudera.org:8080/20343 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I633b5b37ae7555e5622399924bd05cf08d53d853 Gerrit-Change-Number: 20343 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 21 Sep 2023 23:35:13 +0000 Gerrit-HasComments: Yes
