KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/20343 )
Change subject: [multi-tenancy] add tenancy metadata on master ...................................................................... Patch Set 3: (10 comments) Thanks for your reviews. http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/master.proto@305 PS3, Line 305: // The user that owns the tenant. : optional string owner = 8; : : // Tenant disk size limit. : optional int64 tenant_disk_size_limit = 9; : // Tenant row count limit. : optional int64 tenant_row_count_limit = 10; : : // The comment on the Tenant. : optional string comment = 11; > How will these properties be used? The "owner" is used to identify the owner of the tenant and serves as a complement to the tenant name. It is an optional parameter. The "disk size limit" can be used subsequently to restrict a tenant's usage of disk space. The "comment" can be used to provide additional information about the tenant, and it is also an optional parameter. http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog-test.cc File src/kudu/master/sys_catalog-test.cc: http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog-test.cc@479 PS3, Line 479: TEST_F(SysCatalogTest, TestTenantsInfoOperations) { > I guess the validation of adding/update/deleting tanent, for example, check Yes, this patch mainly focuses on the definition and processing of metadata. The related operations for tenants will be implemented in the next patch. http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog-test.cc@486 PS3, Line 486: tenant_id > nit: kTenantId Done http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog-test.cc@514 PS3, Line 514: l.mutable_data()->pb.set_state(SysTenantsEntryPB::REMOVED); > I'm a bit of confused why the state is REMOVED, but not ALTERING. Could you This is my mistake, it has been fixed. http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog-test.cc@529 PS3, Line 529: loader.Reset(); > It would be clearer to move this line after L534? Done http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog-test.cc@558 PS3, Line 558: ASSERT_NE("new_tenant", reader_lock.data().pb.tenant_name()); : ASSERT_NE("test running", reader_lock.data().pb.state_msg()); : ASSERT_NE(SysTenantsEntryPB::RUNNING, reader_lock.data().pb.state()); > Would it better to check the values are something, rather than they are not This is mainly considering the case where there was no value before, writing it this way would be more intuitive. However, considering your suggestion, I will add a direct judgment method. http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog.cc File src/kudu/master/sys_catalog.cc: http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog.cc@1098 PS3, Line 1098: KuduPartialRow row(&schema_); > How about adding a VLOG like ReqAddTenant/ReqUpdateTenant? Done http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/tenant_manager.h File src/kudu/master/tenant_manager.h: http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/tenant_manager.h@55 PS3, Line 55: / This object uses copy-on-write for the portions of data which are persisted : // on disk. This allows the mutated data to be staged and written to disk : // while readers continue to access the previous version. These portions : // of data are in PersistentTenantInfo above, and typically accessed using : // TenantMetadataLock. For example: : // : // TenantInfo* tenant = ...; : // TenantMetadataLock l(tenant, TenantMetadataLock::READ); : // if (l.data().is_running()) { ... } > Similar to class TableInfo, we can simplify the description like "Please se Done http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/tenant_manager.h@73 PS3, Line 73: a > nit: an Done http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/tenant_manager.h@91 PS3, Line 91: // No synchronization needed. > Is it true? I see it is under the lock. The original meaning was that the caller does not need to lock. Since it may cause confusion, I will remove it directly. -- 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: 3 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 29 Aug 2023 12:11:37 +0000 Gerrit-HasComments: Yes
