[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20144 ) Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Reviewed-on: http://gerrit.cloudera.org:8080/20144 Tested-by: Kudu Jenkins Reviewed-by: Yingchun Lai --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 1,018 insertions(+), 297 deletions(-) Approvals: Kudu Jenkins: Verified Yingchun Lai: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 11 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20144 ) Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. Patch Set 10: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 10 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Fri, 28 Jul 2023 06:53:11 + Gerrit-HasComments: No
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20144 ) Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. Patch Set 10: Code-Review+1 LGTM, thanks for the contribution! -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 10 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Thu, 27 Jul 2023 03:59:01 + Gerrit-HasComments: No
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
KeDeng has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 10:
(9 comments)
Thanks for your reviews.
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager-test.cc@244
PS9, Line 244: enable
> nit: enabled
Done
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager-test.cc@253
PS9, Line 253: nullop
> nit: alignment
Done
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.h@604
PS9, Line 604: AddDataDirManager(
> nit: AddDataDirManager
Done
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@666
PS9, Line 666: CHECK(!dd_manager(tenant_id))
> Is it possible the tanent exists when CreateNewDataDirManager? How about to
Done
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@670
PS9, Line 670: _opts.m
> Missing to set 'dir_type'?
Done
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@799
PS9, Line 799: tenant_metadata->set_tenant_key_iv(tenant_key_iv);
: tenant_metadata->set_tenant_key_version(tenant_key_version);
: UpdateMetadataFormatAndStampUnlock(metadata.get());
:
: return UpdateMetadata(metadata);
: }
:
> Reuse UpdateMetadataFormatAndStampUnlock().
Done
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@826
PS9, Line 826:
: Status FsManager::CreateInitialFileSystemLayout(optional
uuid,
: optional
tenant_name,
: optional
tenant_id,
: optional
encryption_key,
: optional
encryption_key_iv,
: optional
encryption_key_version) {
:
> Reuse UpdateMetadataFormatAndStampUnlock().
Done
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@1167
PS9, Line 1167: tenant_id,
> What's the behavior of re-creating a key for the same tanent id?
If we can ensure that the tenant id for accessing KMS is the same, we will
obtain the same key info.
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@1171
PS9, Line 1171: _
> Remove the space. Some other places are the same.
Done
--
To view, visit http://gerrit.cloudera.org:8080/20144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1
Gerrit-Change-Number: 20144
Gerrit-PatchSet: 10
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Wed, 26 Jul 2023 14:15:52 +
Gerrit-HasComments: Yes
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#10). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 1,018 insertions(+), 297 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/10 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 10 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 9:
(9 comments)
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager-test.cc@244
PS9, Line 244: enable
nit: enabled
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager-test.cc@253
PS9, Line 253: nullopt
nit: alignment
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.h@604
PS9, Line 604: AddDdataDirManager
nit: AddDataDirManager
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@666
PS9, Line 666: if (!dd_manager(tenant_id)) {
Is it possible the tanent exists when CreateNewDataDirManager? How about to
CHECK the dd_manager is not exist?
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@670
PS9, Line 670: dm_opts
Missing to set 'dir_type'?
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@799
PS9, Line 799: string time_str;
: StringAppendStrftime(_str, "%Y-%m-%d %H:%M:%S",
time(nullptr), false);
: string hostname;
: if (!GetHostname().ok()) {
: hostname = "";
: }
: metadata->set_format_stamp(Substitute("Formatted at $0 on $1",
time_str, hostname));
Reuse UpdateMetadataFormatAndStampUnlock().
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@826
PS9, Line 826: string time_str;
: StringAppendStrftime(_str, "%Y-%m-%d %H:%M:%S",
time(nullptr), false);
: string hostname;
: if (!GetHostname().ok()) {
: hostname = "";
: }
: metadata->set_format_stamp(Substitute("Formatted at $0 on $1",
time_str, hostname));
:
Reuse UpdateMetadataFormatAndStampUnlock().
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@1167
PS9, Line 1167: GenerateTenantKey
What's the behavior of re-creating a key for the same tanent id?
http://gerrit.cloudera.org:8080/#/c/20144/9/src/kudu/fs/fs_manager.cc@1171
PS9, Line 1171:
Remove the space. Some other places are the same.
--
To view, visit http://gerrit.cloudera.org:8080/20144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1
Gerrit-Change-Number: 20144
Gerrit-PatchSet: 9
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Wed, 26 Jul 2023 13:08:14 +
Gerrit-HasComments: Yes
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#9). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 1,028 insertions(+), 297 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/9 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 9 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
KeDeng has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 8:
(5 comments)
Thanks for your reviews.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@111
PS6, Line 111: public
testing::WithParamInterface> {
: public:
: FsManagerTestBase()
: : fs_root_(GetTestPath("fs_root")) {
: }
> The validator there is to ensure the case of (FLAGS_enable_multi_tenancy &&
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@212
PS6, Line 212:
: private:
> Same to the above, I meant --encrypt_data_at_rest = true and --enable_multi
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@a377
PS6, Line 377:
> Yeah, but is it we expected to create a new Env, in other words, is it poss
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@600
PS6, Line 600: a
> nit: use 'DataDir' to keep consistency.
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@617
PS6, Line 617:
> Can it be constant?
Done
--
To view, visit http://gerrit.cloudera.org:8080/20144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1
Gerrit-Change-Number: 20144
Gerrit-PatchSet: 8
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Wed, 26 Jul 2023 04:28:57 +
Gerrit-HasComments: Yes
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#8). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 1,029 insertions(+), 298 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/8 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 8 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 7:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@111
PS6, Line 111: tenant_id_ = std::get<1>(GetParam());
: if (tenant_id() != kTenantSelectors[0]) {
: FLAGS_encrypt_data_at_rest = true;
: FLAGS_enable_multi_tenancy = true;
: }
> I think it is unnecessary to test this case because there is a dependency a
The validator there is to ensure the case of (FLAGS_enable_multi_tenancy &&
!FLAGS_encrypt_data_at_rest) is invalid, the case I mentioned above is valid,
isn't it?
The case is the Kudu user enabled the encrypt data at rest, but didn't use the
CLI tool you introduced (kudu fs upgrade_encryption_key) to upgrade to
multi-tenancy, but upgrade Kudu to a newer version which includes this patch.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@212
PS6, Line 212: ASSERT_FALSE(FLAGS_encrypt_data_at_rest);
: ASSERT_EQ(0, tenant_num);
> The multi-tenancy feature depends on the encrypt data at rest feature and h
Same to the above, I meant --encrypt_data_at_rest = true and
--enable_multi_tenancy = false (not --encrypt_data_at_rest = false and
--enable_multi_tenancy = true).
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@a377
PS6, Line 377:
> When calling the GetEnv() function, if the tenant does not exist, we do cre
Yeah, but is it we expected to create a new Env, in other words, is it possible
the Env can not be found when calling fs_manager->Exists()?
The non-default tanents and their Envs will be created when opening the fs
manager when the server bootstrap, then everything are prepared. The new tanent
and Env will be created only when calling AddTenant() explicitly IMO. If it is
true, get the Env without creating new one and return nullptr, then CHECK the
Env in Exists().
ListDir(), block_manager() and etc are the same.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@600
PS6, Line 600: DD
nit: use 'DataDir' to keep consistency.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@617
PS6, Line 617: );
Can it be constant?
--
To view, visit http://gerrit.cloudera.org:8080/20144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1
Gerrit-Change-Number: 20144
Gerrit-PatchSet: 7
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Tue, 25 Jul 2023 16:28:25 +
Gerrit-HasComments: Yes
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
KeDeng has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 7:
(20 comments)
Thanks for your reviews.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/cfile/cfile-test.cc@978
PS6, Line 978: auto bm = fs_manager_->bloc
> nit: All places like this can be replaced by 'auto'.
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@288
PS4, Line 288: // Exposes the tenant id.
> I meant
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/data_dirs.cc@273
PS6, Line 273: dd_manager->swap(dm);
> nit: Use swap.
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/file_block_manager.h
File src/kudu/fs/file_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/file_block_manager.h@77
PS6, Line 77: DataDirMan
> Can it be omitted ?
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@111
PS6, Line 111: tenant_id_ = std::get<1>(GetParam());
: if (tenant_id() != kTenantSelectors[0]) {
: FLAGS_encrypt_data_at_rest = true;
: FLAGS_enable_multi_tenancy = true;
: }
> Is it necessary to test the case of
I think it is unnecessary to test this case because there is a dependency added
when declaring the flag. The specific implementation can be found in line 220
of util/env_posix.cc.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@135
PS6, Line 135: AddNonDefaultTan
> Because the default tanent is not needed to add mannaully, how about naming
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@212
PS6, Line 212: ASSERT_FALSE(FLAGS_encrypt_data_at_rest);
: ASSERT_EQ(0, tenant_num);
> If there is a case FLAGS_encrypt_data_at_rest = true and FLAGS_enable_multi
The multi-tenancy feature depends on the encrypt data at rest feature and has
been declared as a dependency in the flag. Therefore, it is not necessary to
include this case.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@230
PS6, Line 230: removi
> nit: removing
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@239
PS6, Line 239: Re-ad
> nit: Re-add
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@a377
PS6, Line 377:
> I guess the reason of you removed the 'const' is GetEnv(tenant_id) is not c
When calling the GetEnv() function, if the tenant does not exist, we do create
a new env, but this situation can only occur in the scenario where the
multi-tenancy feature is enabled. If multi-tenancy is not enabled and we
attempt to retrieve an env for a nonexistent tenant, we will get a null pointer.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@a381
PS6, Line 381:
> The same.
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@346
PS6, Line 346: std::optional tenant_key,
:std::optional tenant_key_iv,
:std::optional tenant_key_version
> Is it possible to use const reference ?
In most scenarios, we only need to specify the tenant name and ID, and let the
KMS generate the key information. This is mainly to accommodate situations
where we do not have the key information when creating the tenant.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@656
PS6, Line 656: manager
> nit: managers
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@474
PS4, Line 474: SearchBlockManager
> I meant
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.cc@1183
PS6, Line 1183: // Make sure env is available for create dd manager.
> nit: Remove one extra blank line.
Done
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.cc@1185
PS6, Line 1185: return Stat
> How will AddTenant() be called, is it safe and graceful to crash directly?
Thank you for your suggestion, I have made the necessary modifications based on
it.
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#7). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 970 insertions(+), 291 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/7 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 7 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 6:
(20 comments)
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/cfile/cfile-test.cc
File src/kudu/cfile/cfile-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/cfile/cfile-test.cc@978
PS6, Line 978: scoped_refptr
nit: All places like this can be replaced by 'auto'.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@288
PS4, Line 288: // Exposes the tenant id.
> Done
I meant
virtual std::string tenant_id() const = 0;
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/data_dirs.cc@273
PS6, Line 273: *dd_manager = dm;
nit: Use swap.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/file_block_manager.h
File src/kudu/fs/file_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/file_block_manager.h@77
PS6, Line 77: kudu::fs::
Can it be omitted ?
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@111
PS6, Line 111: tenant_id_ = std::get<1>(GetParam());
: if (tenant_id() != kTenantSelectors[0]) {
: FLAGS_encrypt_data_at_rest = true;
: FLAGS_enable_multi_tenancy = true;
: }
Is it necessary to test the case of
FLAGS_encrypt_data_at_rest = true;
FLAGS_enable_multi_tenancy = false;
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@135
PS6, Line 135: InitTenantInNeed
Because the default tanent is not needed to add mannaully, how about naming as
'AddNonDefaultTanent()' ?
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@212
PS6, Line 212: ASSERT_FALSE(FLAGS_encrypt_data_at_rest);
: ASSERT_EQ(0, tenant_num);
If there is a case FLAGS_encrypt_data_at_rest = true and
FLAGS_enable_multi_tenancy = false, the tenant_num will be 1, right? So I think
it's needed to add this test case.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@230
PS6, Line 230: remove
nit: removing
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager-test.cc@239
PS6, Line 239: Readd
nit: Re-add
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@a377
PS6, Line 377:
I guess the reason of you removed the 'const' is GetEnv(tenant_id) is not
constant, the latter may create a new Env when the tenant_id is not exist, but
I'm confused is there a real case when the tenant_id is not exist but the
Exists() will be called? Or is it we expect to create a new Env in this case?
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@a381
PS6, Line 381:
The same.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@346
PS6, Line 346: std::optional tenant_key,
:std::optional tenant_key_iv,
:std::optional tenant_key_version
Is it possible to use const reference ?
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.h@656
PS6, Line 656: manager
nit: managers
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@474
PS4, Line 474: ager> SearchBlockM
> Done
I meant
scoped_refptr SearchBlockManager(const std::string&
tenant_id) const
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.cc@1183
PS6, Line 1183:
nit: Remove one extra blank line.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/fs_manager.cc@1185
PS6, Line 1185: CHECK_NOTNULL
How will AddTenant() be called, is it safe and graceful to crash directly?
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/log_block_manager.h@210
PS6, Line 210: kudu::fs::
nit: can be ommited.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/fs/log_block_manager.h@539
PS6, Line 539: kudu::fs::
nit: can be ommited.
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/tablet/delta_compaction.cc
File src/kudu/tablet/delta_compaction.cc:
http://gerrit.cloudera.org:8080/#/c/20144/6/src/kudu/tablet/delta_compaction.cc@239
PS6, Line 239: scoped_refptr
nit: use auto
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
KeDeng has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 5:
(20 comments)
Thanks for your reviews.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@211
PS4, Line 211: enum class MergeReport {
> How about using the C++11 standard enum class ?
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@212
PS4, Line 212: fs report merge
> Merge what kind of operations?
I meant fs report merge operation. I've updated the comments, thanks for your
advices.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@288
PS4, Line 288: // Exposes the tenant id.
> It can be constant?
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.h@122
PS4, Line 122: DataDirMan
> Why add the namespace?
Removed.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.cc@252
PS4, Line 252: dd_manager->swap(dm);
> scoped_refptr has a 'swap()' member function, we can use it as well?
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/dir_manager.h
File src/kudu/fs/dir_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/dir_manager.h@296
PS4, Line 296: const std::string tenant_id() { return opts_.tenant_id; }
> nit: mark it as constant.
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager-test.cc
File src/kudu/fs/error_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager-test.cc@90
PS4, Line 90: u
> nit: use another name.
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h
File src/kudu/fs/error_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h@35
PS4, Line 35: The input string
> Add some comments for the newly added parameter.
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h@149
PS4, Line 149: uuid
> Add some comments of the newly added parameter for the two functions below.
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc@323
PS4, Line 323: NO_FATALS(InitTenantInNeed());
> This line is aim to check the behavior of add duplicate tenant?
To fully validate the logic of the newly added code and check for any potential
defects, I expanded the existing unit tests to cover non-encrypted and
multi-tenant scenarios. The non-encrypted scenario is the current environment,
while the multi-tenant scenario includes testing for additional tenants to
demonstrate the basic functionality of new tenants being the same as the
default tenant. The identification of the corresponding scenario and
determination of whether a new test tenant needs to be added is essential.
But this test is far from enough for testing paths. I will improve this test in
subsequent patches after implementing the path management for multiple tenants.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc@1082
PS4, Line 1082: auto dd_manager = fs_manager(
> How about use 'auto' instead to simplify the code?
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@273
PS4, Line 273: subdirectori
> nit: subdirectories?
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@425
PS4, Line 425: .
> I think it's not necessary to mention the internal member, the developer ma
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@432
PS4, Line 432: metadata
> nit: metadata?
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@474
PS4, Line 474: ager> SearchBlockM
> Different from InitBlockManager, this function is immutable, we can declare
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@596
PS4, Line 596: keye
> nit: keyed by ?
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@658
PS4, Line 658: 'dd_manager_map_' below.
> nit: 'dd_manager_map_'
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@664
PS4, Line 664: 'block_manager_map_'.
> nit: 'block_manager_map_'
Done
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager-test.cc@153
PS4, Line 153:
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#6). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 969 insertions(+), 292 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/6 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 6 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#5). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 969 insertions(+), 292 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/5 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng Gerrit-Reviewer: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 4:
(20 comments)
I didn't review fs_manager.cc carefully this time, I'll do it later.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h
File src/kudu/fs/block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@211
PS4, Line 211: enum MergeReport {
How about using the C++11 standard enum class ?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@212
PS4, Line 212: merge operation
Merge what kind of operations?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/block_manager.h@288
PS4, Line 288: virtual std::string tenant_id() = 0;
It can be constant?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.h
File src/kudu/fs/data_dirs.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.h@122
PS4, Line 122: kudu::fs::
Why add the namespace?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.cc
File src/kudu/fs/data_dirs.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/data_dirs.cc@252
PS4, Line 252: *dd_manager = dm;
scoped_refptr has a 'swap()' member function, we can use it as well?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/dir_manager.h
File src/kudu/fs/dir_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/dir_manager.h@296
PS4, Line 296: std::string tenant_id() { return opts_.tenant_id; }
nit: mark it as constant.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager-test.cc
File src/kudu/fs/error_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager-test.cc@90
PS4, Line 90: s
nit: use another name.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h
File src/kudu/fs/error_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h@35
PS4, Line 35: The input string
Add some comments for the newly added parameter.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/error_manager.h@149
PS4, Line 149: uuid
Add some comments of the newly added parameter for the two functions below.
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc
File src/kudu/fs/fs_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc@323
PS4, Line 323: NO_FATALS(InitTenantInNeed());
This line is aim to check the behavior of add duplicate tenant?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager-test.cc@1082
PS4, Line 1082: scoped_refptr
How about use 'auto' instead to simplify the code?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@273
PS4, Line 273: subdirectory
nit: subdirectories?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@425
PS4, Line 425: from 'dd_manager_map_'
I think it's not necessary to mention the internal member, the developer may be
confused: is there any other function to get dd manager from any other place?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@432
PS4, Line 432: metadate
nit: metadata?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@474
PS4, Line 474: SearchBlockManager
Different from InitBlockManager, this function is immutable, we can declare it
as constant?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@596
PS4, Line 596: with
nit: keyed by ?
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@658
PS4, Line 658: the data dir manager map below
nit: 'dd_manager_map_'
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/fs_manager.h@664
PS4, Line 664: the block manager map below
nit: 'block_manager_map_'
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager-test.cc
File src/kudu/fs/log_block_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager-test.cc@153
PS4, Line 153: std::vectorhttp://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager.cc
File src/kudu/fs/log_block_manager.cc:
http://gerrit.cloudera.org:8080/#/c/20144/4/src/kudu/fs/log_block_manager.cc@
PS4, Line : scoped_refptr
I guess all explicit type declaration like this can be replaced by 'auto'?
--
To view, visit http://gerrit.cloudera.org:8080/20144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1
Gerrit-Change-Number: 20144
Gerrit-PatchSet: 4
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
KeDeng has posted comments on this change. (
http://gerrit.cloudera.org:8080/20144 )
Change subject: [multi-tenancy] add add/remove tenant api in fs layer
..
Patch Set 4:
(15 comments)
Thanks for your reviews.
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/dir_manager.h
File src/kudu/fs/dir_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/dir_manager.h@296
PS2, Line 296: tenant_id
> nit: Mark it as constant.
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager-test.cc
File src/kudu/fs/error_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager-test.cc@90
PS2, Line 90: s
> nit: maybe distinguish the names, although they are not used.
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager.h
File src/kudu/fs/error_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager.h@35
PS2, Line 35: The input string is the UUID a failed
: // component.
> nit: Update the comments.
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h
File src/kudu/fs/fs_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@428
PS2, Line 428: scoped_refptr
> There are many related changes in this patch, could you please charify the
The multi-tenancy feature is implemented based on the data at rest encryption
feature, and the data at rest encryption can be enabled separately. In order
to maximize compatibility between these two different features, different
tenants in the multi-tenant scenario will use different env, and the env of
different tenants will have their own encryption key initialization. To achieve
this goal, the related components such as dd manager are also separated by
tenants and used independently.
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@474
PS2, Line 474: SearchBlockManager
> GetBlockManager? And mark this function as constant?
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@627
PS2, Line 627: env_map
> nit: 'env_map_'
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc
File src/kudu/fs/fs_manager.cc:
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1156
PS2, Line 1156:
> Is it possible to AddTenant() with a same tenant concurrently? It seems blo
In reality, it is unlikely that the same tenant will be added simultaneously
here. Subsequent requests to add new tenants will first go to the master and
then be sent separately to the tserver.
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1212
PS2, Line 1212:
> Maybe return NotFound in this case?
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1226
PS2, Line 1226:
> nit: reserve the size before the loop.
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1232
PS2, Line 1232: == fs:
> There are many related updates (env() to GetEnv()) in this patch, how about
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1238
PS2, Line 1238: if (env) {
> It's a bit of waste to create a shared_ptr if the tenant_id's env can be fo
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1379
PS2, Line 1379:
> Remove the space, other places are the same.
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h
File src/kudu/fs/log_block_manager.h:
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h@520
PS2, Line 520: tenant_id_
> Both file block manager and log block manager have this member, why not mak
Mainly based on the existing implementation, the current file block manager and
log block manager have many common members and have not been put into the super
class BlockManager.
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h@539
PS2, Line 539: kudu::fs::
> nit: can be omitted.
Done
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:
http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/tools/kudu-tool-test.cc@8136
PS2, Line 8136:
> How about spliting the change of updating tanent_name to tanent_id to a sep
Done
--
To view, visit http://gerrit.cloudera.org:8080/20144
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1
Gerrit-Change-Number: 20144
Gerrit-PatchSet: 4
Gerrit-Owner: KeDeng
Gerrit-Reviewer: KeDeng
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai
Gerrit-Comment-Date: Wed, 12 Jul 2023 03:07:24 +
Gerrit-HasComments: Yes
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#4). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 962 insertions(+), 291 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/4 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 4 Gerrit-Owner: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Yingchun Lai, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#3). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h 35 files changed, 963 insertions(+), 291 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/3 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 3 Gerrit-Owner: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20144 ) Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. Patch Set 2: (15 comments) http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/dir_manager.h File src/kudu/fs/dir_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/dir_manager.h@296 PS2, Line 296: tenant_id nit: Mark it as constant. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager-test.cc File src/kudu/fs/error_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager-test.cc@90 PS2, Line 90: s nit: maybe distinguish the names, although they are not used. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager.h File src/kudu/fs/error_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/error_manager.h@35 PS2, Line 35: The input string is the UUID a failed : // component. nit: Update the comments. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h File src/kudu/fs/fs_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@428 PS2, Line 428: scoped_refptr There are many related changes in this patch, could you please charify the reason? Thanks. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@474 PS2, Line 474: SearchBlockManager GetBlockManager? And mark this function as constant? http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.h@627 PS2, Line 627: env map nit: 'env_map_' http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc File src/kudu/fs/fs_manager.cc: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1156 PS2, Line 1156: AddTenant Is it possible to AddTenant() with a same tenant concurrently? It seems block_manager may be overwriten in "block_manager_map_[tenant_id] = block_manager;" http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1212 PS2, Line 1212: NotSupported Maybe return NotFound in this case? http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1226 PS2, Line 1226: push_back nit: reserve the size before the loop. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1232 PS2, Line 1232: GetEnv There are many related updates (env() to GetEnv()) in this patch, how about defining GetEnv() to return the default env_ at first in a separate patch, the patches would be smaller to review then. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1238 PS2, Line 1238: std::shared_ptr It's a bit of waste to create a shared_ptr if the tenant_id's env can be found, return the found env in this case. 'std::' can be omitted. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/fs_manager.cc@1379 PS2, Line 1379: Remove the space, other places are the same. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h@520 PS2, Line 520: tenant_id_ Both file block manager and log block manager have this member, why not make it as a member of super class BlockManager? http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/fs/log_block_manager.h@539 PS2, Line 539: kudu::fs:: nit: can be omitted. http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/tools/kudu-tool-test.cc File src/kudu/tools/kudu-tool-test.cc: http://gerrit.cloudera.org:8080/#/c/20144/2/src/kudu/tools/kudu-tool-test.cc@8136 PS2, Line 8136: kDefaultTenantID How about spliting the change of updating tanent_name to tanent_id to a separate patch? -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 2 Gerrit-Owner: KeDeng Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Tue, 04 Jul 2023 06:48:39 + Gerrit-HasComments: Yes
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20144 to look at the new patch set (#2). Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Moreover, to standardize the use of tenants, I changed the boundary parameter of the multi-tenant related APIs from tenant name to tenant ID. In the future, tenant name updates will be supported, while tenant ID modifications will be prohibited. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/log.cc M src/kudu/consensus/log_reader.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/integration-tests/ts_recovery-itest.cc M src/kudu/kserver/kserver.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/env.h M src/kudu/util/env_posix.cc 49 files changed, 1,143 insertions(+), 424 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/2 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 2 Gerrit-Owner: KeDeng Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [multi-tenancy] add add/remove tenant api in fs layer
KeDeng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20144 Change subject: [multi-tenancy] add add/remove tenant api in fs layer .. [multi-tenancy] add add/remove tenant api in fs layer In this patch, my main addition is the API for adding and deleting tenants in the fs layer. Moreover, to standardize the use of tenants, I changed the boundary parameter of the multi-tenant related APIs from tenant name to tenant ID. In the future, tenant name updates will be supported, while tenant ID modifications will be prohibited. Additionally, I adjusted the env, dd_manager, block_manager, and other relevant parameters to be tenant-related. For the default tenant, which is used for both enabled and not enabled multi-tenant features, the default parameters will be used. New tenants enabled with multi-tenant feature will possess their own unique environment parameters. Also, I added unit tests to ensure the validity of the newly added logic. However, this patch leaves the standardization work for tenant storage paths in multi-tenant scenarios to the next patch, where I will focus on implementing this part. Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 --- M src/kudu/cfile/cfile-test.cc M src/kudu/consensus/consensus_meta.cc M src/kudu/consensus/log.cc M src/kudu/consensus/log_reader.cc M src/kudu/fs/block_manager-stress-test.cc M src/kudu/fs/block_manager-test.cc M src/kudu/fs/block_manager.h M src/kudu/fs/data_dirs-test.cc M src/kudu/fs/data_dirs.cc M src/kudu/fs/data_dirs.h M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/error_manager-test.cc M src/kudu/fs/error_manager.cc M src/kudu/fs/error_manager.h M src/kudu/fs/file_block_manager.cc M src/kudu/fs/file_block_manager.h M src/kudu/fs/fs_manager-test.cc M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/fs/log_block_manager-test.cc M src/kudu/fs/log_block_manager.cc M src/kudu/fs/log_block_manager.h M src/kudu/integration-tests/ts_recovery-itest.cc M src/kudu/kserver/kserver.cc M src/kudu/master/catalog_manager.cc M src/kudu/master/master.cc M src/kudu/tablet/compaction-test.cc M src/kudu/tablet/compaction.cc M src/kudu/tablet/compaction.h M src/kudu/tablet/delta_compaction.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/tablet_bootstrap-test.cc M src/kudu/tablet/tablet_bootstrap.cc M src/kudu/tablet/tablet_metadata.cc M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/tool_action_fs.cc M src/kudu/tools/tool_action_local_replica.cc M src/kudu/tserver/tablet_copy_client-test.cc M src/kudu/tserver/tablet_copy_client.cc M src/kudu/tserver/tablet_copy_service-test.cc M src/kudu/tserver/tablet_copy_source_session.cc M src/kudu/tserver/tablet_copy_source_session.h M src/kudu/tserver/tablet_server-test.cc M src/kudu/tserver/tablet_server.cc M src/kudu/tserver/ts_tablet_manager.cc M src/kudu/tserver/ts_tablet_manager.h M src/kudu/util/env.h M src/kudu/util/env_posix.cc 49 files changed, 1,142 insertions(+), 423 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/44/20144/1 -- To view, visit http://gerrit.cloudera.org:8080/20144 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3f31d3ddd636952f8bd432330afbde018169a2a1 Gerrit-Change-Number: 20144 Gerrit-PatchSet: 1 Gerrit-Owner: KeDeng
