Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20309 )
Change subject: KUDU-3413 [multi-tenancy] Support for tenant storage path isolation ...................................................................... Patch Set 5: Code-Review-1 (1 comment) I tried running a cluster in a clear environment, and I got an error: I20240103 12:56:00.630818 2130065 server_base.cc:767] This appears to be a new deployment of Kudu; creating new FS layout F20240103 12:56:00.631273 2130065 map-util.h:98] Check failed: it != collection.end() Map key not found: 876 *** Check failure stack trace: *** Signal: SIGABRT (Aborted) the relevant command line flags are: --enable_multi_tenancy=true --encrypt_data_at_rest=true --unlock_experimental_flags --test-tenant-id=876 --test-tenant-key=test --test-tenant-name=test --test-tenant-key-iv=test --test-tenant-key-version=1" I understand that the tenant directory doesn't exist yet, but I think it should create it instead of throwing an error. http://gerrit.cloudera.org:8080/#/c/20309/5/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/20309/5/src/kudu/fs/fs_manager.h@283 PS5, Line 283: std::string GetWalsRootDir( This is a general comment rather than a comment about this special line. I think setting tenant at multiple lines is an antipattern, and if tenant_id will be used by then every place where these functions are called, tenant_id should be needed to pass on. I have two possible ideas: 1. create a subclass from FSManager that handles and encapsulates tenant data, and that object will be passed down to the objects that use them. 2. If I understand correctly there can be a tablet_id->tenant_id mapping, and tablet_id is already propagated to everywhere, and getting the tenant_id from some kind of map instead of as an argument would make the code less complex. (I checked if GetWalsRootDir can get a tablet_id instead of tenant_id, and it looks like it is possible with one exception: int64_t wal_dir_space = CalculateAvailableSpace(options, fs_manager.GetWalsRootDir(), FLAGS_fs_wal_dir_reserved_bytes, &space_info_waldir); which raises the question: when any kind of available space calculation happens, should we calculate all of the tenants together? Should they be calculated separately? My intuition that they should be calculated together. ) -- To view, visit http://gerrit.cloudera.org:8080/20309 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Icb3f3d47e00f93db6f1ce66596de6449fb1dbb73 Gerrit-Change-Number: 20309 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Ádám Bakai <[email protected]> Gerrit-Comment-Date: Wed, 03 Jan 2024 12:44:46 +0000 Gerrit-HasComments: Yes
