KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/20343 )
Change subject: KUDU-3413[multi-tenancy] add tenancy metadata on master ...................................................................... Patch Set 6: (7 comments) Thank you for your review comments. I have made the necessary modifications based on your feedback. If possible, could you please help me review it again? Thank you very much. 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 Thank you for your correction. When the multi-tenancy feature was initially developed, I included the JIRA number in the patch title. Later, someone pointed out that we should also include a feature label. So, I added the JIRA number and feature label to the patch title. Then, someone commented that we should pay attention to the length of the title. In order to meet this requirement, I removed the JIRA number. After that, no one made any further comments on the title. So, in subsequent developments, I only kept the feature label. Based on your suggestion, I will prioritize the JIRA number and include the feature label if the title length allows. Do you think this approach is feasible? 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: // Used to indicate the lifecycle of the current tenant. : enum State { : UNKNOWN = 0; : PREPARING = 1; : RUNNING = 2; : ALTERING = 3; : > Please add a comment to explain what each 'State' is about. Done http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@288 PS5, Line 288: // Tenan > Here and below: please don't use 'required' for newly introduced PB fields Done http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@288 PS5, Line 288: > Here and below: why to add 'tenant_' prefix for these fields? Also, the c Done http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@294 PS5, Line 294: rsion. > If that's a version, why not to use a numeric type instead of a string inst This is mainly to maintain consistency with the definition in the fs layer." http://gerrit.cloudera.org:8080/#/c/20343/5/src/kudu/master/master.proto@300 PS5, Line 300: > nit: creation Done 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: for tenant. > There is anything below, so maybe drop this lock at all? Done -- 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: 6 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: Fri, 22 Sep 2023 03:22:28 +0000 Gerrit-HasComments: Yes
