You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "KeDeng (Code Review)" <ge...@cloudera.org> on 2023/08/11 06:57:42 UTC

[kudu-CR] [multi-tenancy] add tenancy metadata on master

KeDeng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20343


Change subject: [multi-tenancy] add tenancy metadata on master
......................................................................

[multi-tenancy] add tenancy metadata on master

We have made initial progress in implementing multi-tenancy
at the fs layer, but there is still a long way to go for
complete implementation of multi-tenancy features. The most
important thing at the moment is to improve the management
of multi-tenancy on this basis.

For this part, my preliminary plan is to reference the
management implementation of tables and store the metadata
of tenants on the master, with the leader of the master
handling the relevant processes.

To achieve this goal, I have added tenant metadata on the
master in this patch and implemented basic metadata operations.
At the same time, I have added unit tests for certain logic to
ensure the correctness of the newly added logic.

Change-Id: I633b5b37ae7555e5622399924bd05cf08d53d853
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/catalog_manager.h
M src/kudu/master/master.proto
M src/kudu/master/sys_catalog-test.cc
M src/kudu/master/sys_catalog.cc
M src/kudu/master/sys_catalog.h
6 files changed, 467 insertions(+), 2 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/43/20343/1
-- 
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: newchange
Gerrit-Change-Id: I633b5b37ae7555e5622399924bd05cf08d53d853
Gerrit-Change-Number: 20343
Gerrit-PatchSet: 1
Gerrit-Owner: KeDeng <kd...@gmail.com>

[kudu-CR] [multi-tenancy] add tenancy metadata on master

Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.

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 <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <la...@apache.org>
Gerrit-Comment-Date: Thu, 21 Sep 2023 23:35:13 +0000
Gerrit-HasComments: Yes