Yingchun Lai 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) 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? 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 adding duplicated tanent, modify tanent id, and etc, will be implemented in another patch, right? http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/sys_catalog-test.cc@486 PS3, Line 486: tenant_id nit: kTenantId 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 please clarify about this? 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? 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 something? If checking they are old values, they are not the new values nor any other values. 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? 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 see the TabletInfo class doc above for more information." to reduce duplciation. http://gerrit.cloudera.org:8080/#/c/20343/3/src/kudu/master/tenant_manager.h@73 PS3, Line 73: a nit: an 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. -- 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: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 29 Aug 2023 03:46:03 +0000 Gerrit-HasComments: Yes
