[kudu-CR](branch-1.17.x) KUDU-3476: Update 1.17 release notes
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20242 ) Change subject: KUDU-3476: Update 1.17 release notes .. KUDU-3476: Update 1.17 release notes This patch udpates the release notes for the 1.17 release to include the range aware replica placement feature. Change-Id: I430cf540731860ec9209f5c6026cdc4431a3d2bf Reviewed-on: http://gerrit.cloudera.org:8080/20242 Reviewed-by: Yingchun Lai Tested-by: Kudu Jenkins --- M docs/release_notes.adoc 1 file changed, 5 insertions(+), 0 deletions(-) Approvals: Yingchun Lai: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/20242 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: merged Gerrit-Change-Id: I430cf540731860ec9209f5c6026cdc4431a3d2bf Gerrit-Change-Number: 20242 Gerrit-PatchSet: 3 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR](branch-1.17.x) [doc] Update release notes.adoc
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20260 ) Change subject: [doc] Update release_notes.adoc .. Patch Set 3: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I26a793367c0c5253a732c090a160a89d72fc215a Gerrit-Change-Number: 20260 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Wed, 26 Jul 2023 05:33:21 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) [doc] Update release notes.adoc
Yingchun Lai has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/20260 ) Change subject: [doc] Update release_notes.adoc .. [doc] Update release_notes.adoc Add KUDU-3489 in the release_notes.adoc under fixed issues. Change-Id: I26a793367c0c5253a732c090a160a89d72fc215a Reviewed-on: http://gerrit.cloudera.org:8080/20260 Tested-by: Kudu Jenkins Reviewed-by: Yingchun Lai --- M docs/release_notes.adoc 1 file changed, 5 insertions(+), 0 deletions(-) Approvals: Kudu Jenkins: Verified Yingchun Lai: Looks good to me, approved -- To view, visit http://gerrit.cloudera.org:8080/20260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: merged Gerrit-Change-Id: I26a793367c0c5253a732c090a160a89d72fc215a Gerrit-Change-Number: 20260 Gerrit-PatchSet: 4 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR](branch-1.17.x) KUDU-3476: Update 1.17 release notes
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20242 ) Change subject: KUDU-3476: Update 1.17 release notes .. Patch Set 2: Code-Review+2 -- To view, visit http://gerrit.cloudera.org:8080/20242 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I430cf540731860ec9209f5c6026cdc4431a3d2bf Gerrit-Change-Number: 20242 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Wed, 26 Jul 2023 05:33:04 + Gerrit-HasComments: No
[kudu-CR](branch-1.17.x) KUDU-3476: Update 1.17 release notes
Yingchun Lai has uploaded a new patch set (#2) to the change originally created by Mahesh Reddy. ( http://gerrit.cloudera.org:8080/20242 ) Change subject: KUDU-3476: Update 1.17 release notes .. KUDU-3476: Update 1.17 release notes This patch udpates the release notes for the 1.17 release to include the range aware replica placement feature. Change-Id: I430cf540731860ec9209f5c6026cdc4431a3d2bf --- M docs/release_notes.adoc 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/42/20242/2 -- To view, visit http://gerrit.cloudera.org:8080/20242 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: newpatchset Gerrit-Change-Id: I430cf540731860ec9209f5c6026cdc4431a3d2bf Gerrit-Change-Number: 20242 Gerrit-PatchSet: 2 Gerrit-Owner: Mahesh Reddy Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai
[kudu-CR](branch-1.17.x) [doc] Update release notes.adoc
Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20260 ) Change subject: [doc] Update release_notes.adoc .. Patch Set 3: (1 comment) > Patch Set 2: > > (1 comment) http://gerrit.cloudera.org:8080/#/c/20260/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20260/2//COMMIT_MSG@8 PS2, Line 8: > nit: Add a blank line between the title and message. Done -- To view, visit http://gerrit.cloudera.org:8080/20260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: comment Gerrit-Change-Id: I26a793367c0c5253a732c090a160a89d72fc215a Gerrit-Change-Number: 20260 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai Gerrit-Comment-Date: Wed, 26 Jul 2023 05:32:27 + Gerrit-HasComments: Yes
[kudu-CR](branch-1.17.x) [doc] Update release notes.adoc
Yingchun Lai has uploaded a new patch set (#3) to the change originally created by Abhishek Chennaka. ( http://gerrit.cloudera.org:8080/20260 ) Change subject: [doc] Update release_notes.adoc .. [doc] Update release_notes.adoc Add KUDU-3489 in the release_notes.adoc under fixed issues. Change-Id: I26a793367c0c5253a732c090a160a89d72fc215a --- M docs/release_notes.adoc 1 file changed, 5 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/60/20260/3 -- To view, visit http://gerrit.cloudera.org:8080/20260 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: branch-1.17.x Gerrit-MessageType: newpatchset Gerrit-Change-Id: I26a793367c0c5253a732c090a160a89d72fc215a Gerrit-Change-Number: 20260 Gerrit-PatchSet: 3 Gerrit-Owner: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin 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 (#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] [tserver] KUDU-1827: tserver decommission
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/18441 )
Change subject: [tserver] KUDU-1827: tserver decommission
..
Patch Set 12:
(11 comments)
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/auto_rebalancer.cc
File src/kudu/master/auto_rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/auto_rebalancer.cc@478
PS12, Line 478: ts_manager_->GetDecommissionedCount()
Does it mean it's OK to put replicas on tablet servers being de-commissioned?
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/master.proto
File src/kudu/master/master.proto:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/master.proto@1042
PS12, Line 1042: At any time only one TS should be in this state to avoid
resource
: // contention and unnecessary moves when decommissioning
multiple instances.
Is this invariant enforced in the code?
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/ts_manager.cc
File src/kudu/master/ts_manager.cc:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/master/ts_manager.cc@227
PS12, Line 227: for (auto const& entry : stateMap) {
: // TServerStateMap entry schema is: >
You could use structure binding for this instead: Kudu code is using C++17
internally.
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.h
File src/kudu/rebalance/rebalancer.h:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.h@219
PS12, Line 219: std::string
This could return 'const std::string&' to avoid useless copying.
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc
File src/kudu/rebalance/rebalancer.cc:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc@539
PS12, Line 539: std::string Rebalancer::RunStatusAsString(Rebalancer::RunStatus
run_status) {
: static const char *enum_str[] =
: { "Unknown", "Cluster is balanced", "Timed out" };
The standard way of doing this is using switch/case statement -- it eliminates
many issues that another approach you use introduces.
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc@543
PS12, Line 543: string tmp
nit: there is no need for this cast from char* to string
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/rebalance/rebalancer.cc@543
PS12, Line 543: run_status
What if this is out of the range of the enum_str array?
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/rebalancer_tool.cc
File src/kudu/tools/rebalancer_tool.cc:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/rebalancer_tool.cc@1625
PS12, Line 1625: maintenance mode
How does this relate to the decommissioning? Is this message still actionable
with the recent updates?
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_common.cc
File src/kudu/tools/tool_action_common.cc:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_common.cc@316
PS12, Line 316: const char* const kTServerAddressesArg = "tserver_addresses";
: const char* const kTServerAddressesDesc = "Comma-separated list
of Kudu Tablet Servers where each "
: "address is of form
'hostname:port'.";
This seems to be a duplicate of what's currently in tool_action_master.cc --
the only difference is the separator. Maybe, consolidate that by just moving
the corresponding definitions from tool_action_master.cc into this file instead?
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_tserver.cc
File src/kudu/tools/tool_action_tserver.cc:
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_tserver.cc@19
PS12, Line 19:
nit: remove the extra empty line
http://gerrit.cloudera.org:8080/#/c/18441/12/src/kudu/tools/tool_action_tserver.cc@74
PS12, Line 74: UUIDs of tablet servers
In what form and what's the separator?
--
To view, visit http://gerrit.cloudera.org:8080/18441
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I15c52b653c20b2a3a45fbf8934f19f6bd1a9caea
Gerrit-Change-Number: 18441
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Chovan
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Zoltan Chovan
Gerrit-Comment-Date: Tue, 25 Jul 2023 22:10:11 +
Gerrit-HasComments: Yes
[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] WIP [fs] improve rolling back of the initial FS layout
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20262 ) Change subject: WIP [fs] improve rolling back of the initial FS layout .. Patch Set 3: > > Hi, I was trying to change my commit message by your style. And > > copied your changeID by mistake. > > So sorry for that, could it be fixed? > > Ah, no problem :) > > Just remove the Change-Id line from the commit message of your > patch, and git will generate a new one for you. Once a new > Change-Id is generated by git, you can re-submit your patch to > gerrit. > > You can edit the commit message by running `git commit --amend`, > and there you'll get the message in your editor session. > > Thank you for your continued contributions! It seems you already addressed the issue and posted a new patch at https://gerrit.cloudera.org/#/c/20264/ So, all is well :) -- To view, visit http://gerrit.cloudera.org:8080/20262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 Gerrit-Change-Number: 20262 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 25 Jul 2023 15:41:10 + Gerrit-HasComments: No
[kudu-CR] WIP [fs] improve rolling back of the initial FS layout
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20262 ) Change subject: WIP [fs] improve rolling back of the initial FS layout .. Patch Set 3: > Hi, I was trying to change my commit message by your style. And > copied your changeID by mistake. > So sorry for that, could it be fixed? Ah, no problem :) Just remove the Change-Id line from the commit message of your patch, and git will generate a new one for you. Once a new Change-Id is generated by git, you can re-submit your patch to gerrit. You can edit the commit message by running `git commit --amend`, and there you'll get the message in your editor session. Thank you for your continued contributions! -- To view, visit http://gerrit.cloudera.org:8080/20262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 Gerrit-Change-Number: 20262 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 25 Jul 2023 15:40:11 + Gerrit-HasComments: No
[kudu-CR] WIP [fs] improve rolling back of the initial FS layout
Hello Kudu Jenkins, Abhishek Chennaka, Ádám Bakai, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20262 to look at the new patch set (#3). Change subject: WIP [fs] improve rolling back of the initial FS layout .. WIP [fs] improve rolling back of the initial FS layout This patch allows for less manual intervention when adding an extra Kudu master into a cluster fails because of an error during creation of the initial FS layout. The rollback logic is engaging only on first run, when no existing FS layout is present yet. As for testing, the already existing TestSomeMastersUnreachable scenario of the AutoAddMasterTest has been updated correspondingly. WIP: * collect initial feedback * clean up the code a bit * maybe, add more tests to cover various failure modes * make sure the logic isn't engaging on already existing FS, e.g. when there is some corruption on existing file or so Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 --- M src/kudu/fs/data_dirs.cc M src/kudu/fs/dir_manager.cc M src/kudu/fs/dir_manager.h M src/kudu/fs/fs_manager.cc M src/kudu/fs/fs_manager.h M src/kudu/master/dynamic_multi_master-test.cc M src/kudu/master/master.cc M src/kudu/master/sys_catalog.cc M src/kudu/server/server_base.cc M src/kudu/server/server_base.h 10 files changed, 173 insertions(+), 70 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/20262/3 -- To view, visit http://gerrit.cloudera.org:8080/20262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 Gerrit-Change-Number: 20262 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20262 ) Change subject: [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1. .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/20262/1/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: PS1: > If we start an existing server with data but add a new empty disk to the co The rollback logic works only on the first start, so it's not going to engage in other scenarios with some data has been present. But I'll add a test -- thanks for pointing at that. -- To view, visit http://gerrit.cloudera.org:8080/20262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 Gerrit-Change-Number: 20262 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 25 Jul 2023 15:32:33 + Gerrit-HasComments: Yes
[kudu-CR] [tools] KUDU-3487 rebalancer checks the voter num.
Song Jiacheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/20264 ) Change subject: [tools] KUDU-3487 rebalancer checks the voter num. .. Patch Set 2: Hi, committers, could you please help me review this? There is more information on JIRA. -- To view, visit http://gerrit.cloudera.org:8080/20264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3b3d5dfb383719f56a2d32b62d2b659b17928794 Gerrit-Change-Number: 20264 Gerrit-PatchSet: 2 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Comment-Date: Tue, 25 Jul 2023 08:50:51 + Gerrit-HasComments: No
[kudu-CR] [tserver] KUDU-1827: tserver decommission
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/18441 to look at the new patch set (#12). Change subject: [tserver] KUDU-1827: tserver decommission .. [tserver] KUDU-1827: tserver decommission This change introduces two new states for TServers for decommissioning. The states are: * DECOMMISSIONING_IN_PROGRESS * DECOMMISSIONED In these states the TServer is quieced and new replicas are prevented to be placed on them. When a TServer is being decommissioned, first it enters the DECOMMISSIONING_IN_PROGRESS state, then a rebalancer job is started to move replicas away from every TServer that is in this state. When the rebalancing is done, the TServers move to DECOMMISSIONED state and will stay in it until they are removed from the cluster via the 'kudu tserver unregister' tool. The decommissioning can be started with the following cli command: $ kudu tserver state enter_decommissioning [-allow_missing_tserver] [-negotiation_timeout_ms=] [-timeout_ms=] It is possible to decommission multiple TServers at once by providing multiple addresses in a comma-separated list, which is preferred to avoid multiple replica moves between each server decommission. Change-Id: I15c52b653c20b2a3a45fbf8934f19f6bd1a9caea --- M src/kudu/master/auto_rebalancer-test.cc M src/kudu/master/auto_rebalancer.cc M src/kudu/master/master.proto M src/kudu/master/master_service.cc M src/kudu/master/ts_manager.cc M src/kudu/master/ts_manager.h M src/kudu/master/ts_state-test.cc M src/kudu/rebalance/rebalancer.cc M src/kudu/rebalance/rebalancer.h M src/kudu/tools/kudu-tool-test.cc M src/kudu/tools/rebalancer_tool.cc M src/kudu/tools/tool_action_common.cc M src/kudu/tools/tool_action_common.h M src/kudu/tools/tool_action_tserver.cc 14 files changed, 547 insertions(+), 19 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/18441/12 -- To view, visit http://gerrit.cloudera.org:8080/18441 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I15c52b653c20b2a3a45fbf8934f19f6bd1a9caea Gerrit-Change-Number: 18441 Gerrit-PatchSet: 12 Gerrit-Owner: Zoltan Chovan Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Zoltan Chovan
[kudu-CR] KUDU-3407 not always flush even if under memory pressure.
Hello Tidy Bot, Alexey Serbin, Ashwani Raina, Kudu Jenkins, Wang Xixu, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20166 to look at the new patch set (#9). Change subject: KUDU-3407 not always flush even if under memory pressure. .. KUDU-3407 not always flush even if under memory pressure. This patch add an argument to give a chance to do other ops while server is under memory pressure. This mechanism works when the memory usage between memory_pressure_percentage and memory_limit_soft_percentage. The higher memory usage is, this higher probability to do flush MRS/DMS. e.g. memory_pressure_percentage = 60% memory_limit_soft_percentage = 80% The probability of not flushing MRS/DMS is the value of not_flush_memory_prob, which is 0.2 by default, when the memory usage is 60%. As the memory increases, it gradually decreases to 0, when the memory usage is 80%. Change-Id: Idc2fd3a850cf99d54ef2980211b712468440ed80 --- M src/kudu/util/maintenance_manager-test.cc M src/kudu/util/maintenance_manager.cc M src/kudu/util/maintenance_manager.h 3 files changed, 103 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/66/20166/9 -- To view, visit http://gerrit.cloudera.org:8080/20166 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: Idc2fd3a850cf99d54ef2980211b712468440ed80 Gerrit-Change-Number: 20166 Gerrit-PatchSet: 9 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
[kudu-CR] KUDU-3407: Give a chance to do other maintenance operations while server is under memory pressure.
Song Jiacheng has posted comments on this change. (
http://gerrit.cloudera.org:8080/20166 )
Change subject: KUDU-3407: Give a chance to do other maintenance operations
while server is under memory pressure.
..
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/20166/7/src/kudu/util/maintenance_manager-test.cc
File src/kudu/util/maintenance_manager-test.cc:
http://gerrit.cloudera.org:8080/#/c/20166/7/src/kudu/util/maintenance_manager-test.cc@922
PS7, Line 922: TEST_F(MaintenanceManagerTest, TestNotFlushMemory) {
> Did you run any practical workloads to see other maintenance ops in action
Thank you for your review!
Yes, I did. This patch has worked in our clusters for a long time. Maintenance
manager does schedule some operations other than flush ops while under memory
pressure.
I have a test, which is involved in KUDU-3488, testing various policies of
maintenance manager, including this not_flush_memory_prob. And it shows that
the not-flush mechanism works.
Sometimes the memory usage of tablet servers stay at about 60%(memory pressure
threshold), because the write workload and the flush ability of maintenance
manager are almost equal, in which case the high perf score operations can not
be run. And eventually the performance of the tablet server will be lower and
lower, leading to higher memory usage, and finally a vicious circle appears.
A user might want to turn on this if he find that the situation which is
described above occurred, and he need to find the balance between the
performance and memory.
Actually, The probability is decreasing while the memory usage is getting close
to the 80%(memory soft limit), so mostly the memory usage won't be too high.
http://gerrit.cloudera.org:8080/#/c/20166/7/src/kudu/util/maintenance_manager.cc
File src/kudu/util/maintenance_manager.cc:
http://gerrit.cloudera.org:8080/#/c/20166/7/src/kudu/util/maintenance_manager.cc@104
PS7, Line 104: DEFINE_double(not_flush_memory_prob, 0,
> You solution only distinguishes the memory related ops and non-memory relat
Thanks for your comment!
I think the ops which we want to run have already got a high perf score, the
only reason they can't be run is that FindBestOp always do flush ops if under
memory pressure. For now, I think the perf score mechanism is able to find the
best op to run after tuning the configurations and table priorities.
http://gerrit.cloudera.org:8080/#/c/20166/7/src/kudu/util/maintenance_manager.cc@104
PS7, Line 104: 0
> If this is going to be 0(i.e. DRS/MRS flush ops will be scheduled as per pr
Exactly, I will commit another patch with more information.
Thanks!
--
To view, visit http://gerrit.cloudera.org:8080/20166
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Idc2fd3a850cf99d54ef2980211b712468440ed80
Gerrit-Change-Number: 20166
Gerrit-PatchSet: 8
Gerrit-Owner: Song Jiacheng
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Song Jiacheng
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Wang Xixu <1450306...@qq.com>
Gerrit-Comment-Date: Tue, 25 Jul 2023 07:33:19 +
Gerrit-HasComments: Yes
[kudu-CR] [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1.
Song Jiacheng has posted comments on this change. ( http://gerrit.cloudera.org:8080/20262 ) Change subject: [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1. .. Patch Set 2: Hi, I was trying to change my commit message by your style. And copied your changeID by mistake. So sorry for that, could it be fixed? -- To view, visit http://gerrit.cloudera.org:8080/20262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 Gerrit-Change-Number: 20262 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 25 Jul 2023 07:27:40 + Gerrit-HasComments: No
[kudu-CR] [tools] KUDU-3487 rebalancer checks the voter num.
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20264 to look at the new patch set (#2). Change subject: [tools] KUDU-3487 rebalancer checks the voter num. .. [tools] KUDU-3487 rebalancer checks the voter num. Replace rebalance for the tables whose replication factor is 1 might stuck for a long time because the rebalancer send LeaderStepDown request even if there is no other voter to transfer leadership. So check the voter num before sending the request. Change-Id: I3b3d5dfb383719f56a2d32b62d2b659b17928794 --- M src/kudu/tools/tool_replica_util.cc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/20264/2 -- To view, visit http://gerrit.cloudera.org:8080/20264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I3b3d5dfb383719f56a2d32b62d2b659b17928794 Gerrit-Change-Number: 20264 Gerrit-PatchSet: 2 Gerrit-Owner: Song Jiacheng Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1.
Song Jiacheng has uploaded a new patch set (#2) to the change originally created by Alexey Serbin. ( http://gerrit.cloudera.org:8080/20262 ) Change subject: [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1. .. [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1. Replace rebalance for the tables whose replication factor is 1 might stuck for a long time because the rebalancer send LeaderStepDown request even if there is no other voter to transfer leadership. So check the voter num before sending the request. Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 --- M src/kudu/tools/tool_replica_util.cc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/62/20262/2 -- To view, visit http://gerrit.cloudera.org:8080/20262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 Gerrit-Change-Number: 20262 Gerrit-PatchSet: 2 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Song Jiacheng Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1. Replace rebalance for the tables whose replication factor is 1 might stuck for a long time because the r
Song Jiacheng has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20264 Change subject: [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1. Replace rebalance for the tables whose replication factor is 1 might stuck for a long time because the rebalancer send LeaderStepDown request even if there is no other v .. [tools] KUDU-3487 Rebalancer do not send LeaderStepDown request if voter number is 1. Replace rebalance for the tables whose replication factor is 1 might stuck for a long time because the rebalancer send LeaderStepDown request even if there is no other voter to transfer leadership. So check the voter num before sending the request. Change-Id: I3b3d5dfb383719f56a2d32b62d2b659b17928794 --- M src/kudu/tools/tool_replica_util.cc 1 file changed, 3 insertions(+), 1 deletion(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/20264/1 -- To view, visit http://gerrit.cloudera.org:8080/20264 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I3b3d5dfb383719f56a2d32b62d2b659b17928794 Gerrit-Change-Number: 20264 Gerrit-PatchSet: 1 Gerrit-Owner: Song Jiacheng
[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] WIP [fs] improve rolling back of the initial FS layout
Abhishek Chennaka has posted comments on this change. ( http://gerrit.cloudera.org:8080/20262 ) Change subject: WIP [fs] improve rolling back of the initial FS layout .. Patch Set 1: (1 comment) http://gerrit.cloudera.org:8080/#/c/20262/1/src/kudu/server/server_base.cc File src/kudu/server/server_base.cc: PS1: If we start an existing server with data but add a new empty disk to the config, with this approach do we end up deleting the data from the all existing disks if there is a failure creating the filesystem in the new disk. I guess this affects the tablet servers as well? -- To view, visit http://gerrit.cloudera.org:8080/20262 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I92821416ca44cde79524b08b49f642dd59102f62 Gerrit-Change-Number: 20262 Gerrit-PatchSet: 1 Gerrit-Owner: Alexey Serbin Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 25 Jul 2023 06:02:39 + Gerrit-HasComments: Yes
