You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2021/07/15 02:56:51 UTC
[kudu-CR] [catalog manager] introduce --min num replicas flag
Alexey Serbin has uploaded this change for review. ( http://gerrit.cloudera.org:8080/17684
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
[catalog_manager] introduce --min_num_replicas flag
This patch introduces a flag for Kudu master to enforce the minimum
number of replicas for newly created tables in a Kudu cluster. Unless
overridden, --min_num_replicas is set to 1 by default.
For example, setting --min_num_replicas=3 enforces every new table to
have at least 3 replicas for each of its tablets, so there cannot be
a data loss when a single tablet server fails irrecoverably.
I also added validators for the related flags to keep things more
consistent and report misconfiguration errors as early as possible.
This patch also contains a few test scenarios to cover the newly
introduced functionality.
Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 182 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/17684/1
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 1
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17684
to look at the new patch set (#3).
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
[catalog_manager] introduce --min_num_replicas flag
This patch introduces a flag for kudu-master to enforce the minimum
number of replicas for newly created tables in a cluster. Unless
overridden, --min_num_replicas is set to 1 by default.
For example, setting --min_num_replicas=3 enforces every new table to
have at least 3 replicas for each of its tablets, so there cannot be
a data loss when a single tablet server in the cluster fails
irrecoverably.
I also added validators for the related flags to keep things more
consistent and report misconfiguration as early as possible. This patch
also contains a few test scenarios to cover the newly introduced
functionality.
Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 186 insertions(+), 40 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/17684/3
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 3: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 15 Jul 2021 14:46:48 +0000
Gerrit-HasComments: No
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 4: Verified+1
unrelated test failures in
* ThreadPoolTest.QueueLoadMeter
* RaftConsensusITest.TestSlowFollower
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 16 Jul 2021 06:47:32 +0000
Gerrit-HasComments: No
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc@5372
PS3, Line 5372: );
nit: maybe print the error, now that we're not going to see it with the ASSERT_STR_CONTAINS()
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@479
PS3, Line 479: if (FLAGS_min_num_replicas % 2 == 0 &&
: !FLAGS_allow_unsafe_replication_factor) {
: LOG(ERROR) << Substitute(
: "--min_num_replicas ($0) must not be an even number since "
: "--allow_unsafe_replication_factor is not set",
: FLAGS_min_num_replicas);
: return false;
: }
: if (FLAGS_max_num_replicas % 2 == 0 &&
: !FLAGS_allow_unsafe_replication_factor) {
: LOG(ERROR) << Substitute(
: "--max_num_replicas ($0) must not be an even number since "
: "--allow_unsafe_replication_factor is not set",
: FLAGS_max_num_replicas);
: return false;
: }
It seems more natural to check that --default_num_replicas for evenness, rather than the min/max values. I.e. it seems acceptable for the min/max to be non-valid RFs but at the very least the default RF should be valid.
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@497
PS3, Line 497: should
> I believe it would lead to a better user experience, yes.
+1
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 15 Jul 2021 17:33:38 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has removed a vote on this change.
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Removed Verified-1 by Kudu Jenkins (120)
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17684
to look at the new patch set (#4).
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
[catalog_manager] introduce --min_num_replicas flag
This patch introduces a flag for kudu-master to enforce the minimum
number of replicas for newly created tables in a cluster. Unless
overridden, --min_num_replicas is set to 1 by default.
For example, setting --min_num_replicas=3 enforces every new table to
have at least 3 replicas for each of its tablets, so there cannot be
a data loss when a single tablet server in the cluster fails
irrecoverably.
I also added validators for the related flags to keep things more
consistent and report misconfiguration as early as possible.
This patch contains a few test scenarios to cover the newly introduced
functionality.
Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 228 insertions(+), 59 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/17684/4
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 3:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc@5359
PS3, Line 5359: {2, "illegal replication factor 2: replication factor must be odd"},
> A few reasons: (1) it's less symbols with a semicolon (2) it's inline with
Done
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@487
PS3, Line 487: if (FLAGS_max_num_replicas % 2 == 0 &&
> I don't see a duplicate: at line 479 the check is against FLAGS_min_num_rep
I don't know how I didn't see that, sorry...
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@497
PS3, Line 497: should
> The following approach is used here: "must" is when it have to be so unless
I believe it would lead to a better user experience, yes.
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 15 Jul 2021 14:46:43 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Attila Bukor, Kudu Jenkins, Andrew Wong, Grant Henke,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/17684
to look at the new patch set (#2).
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
[catalog_manager] introduce --min_num_replicas flag
This patch introduces a flag for kudu-master to enforce the minimum
number of replicas for newly created tables in a cluster. Unless
overridden, --min_num_replicas is set to 1 by default.
For example, setting --min_num_replicas=3 enforces every new table to
have at least 3 replicas for each of its tablets, so there cannot be
a data loss when a single tablet server in the cluster fails
irrecoverably.
I also added validators for the related flags to keep things more
consistent and report misconfiguration as early as possible. This patch
also contains a few test scenarios to cover the newly introduced
functionality.
Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 182 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/84/17684/2
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 2
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc@5356
PS3, Line 5356: const vector<pair<int, string>> cases = {
can't this be expanded with the new test cases instead of creating separate tests?
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc@5359
PS3, Line 5359: {2, "illegal replication factor 2: replication factor must be odd"},
is there any reason for changing parentheses for colons?
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@487
PS3, Line 487: if (FLAGS_max_num_replicas % 2 == 0 &&
is this check duplicated?
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@497
PS3, Line 497: should
nit: here and below, "should" should be replaced with "must"
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 15 Jul 2021 12:57:57 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc@5372
PS3, Line 5372: );
> nit: maybe print the error, now that we're not going to see it with the ASS
Done
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@487
PS3, Line 487: if (FLAGS_max_num_replicas % 2 == 0 &&
> I don't know how I didn't see that, sorry...
ah, that's not a problem at all -- I thought I might be missing something :)
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@479
PS3, Line 479: if (FLAGS_min_num_replicas % 2 == 0 &&
: !FLAGS_allow_unsafe_replication_factor) {
: LOG(ERROR) << Substitute(
: "--min_num_replicas ($0) must not be an even number since "
: "--allow_unsafe_replication_factor is not set",
: FLAGS_min_num_replicas);
: return false;
: }
: if (FLAGS_max_num_replicas % 2 == 0 &&
: !FLAGS_allow_unsafe_replication_factor) {
: LOG(ERROR) << Substitute(
: "--max_num_replicas ($0) must not be an even number since "
: "--allow_unsafe_replication_factor is not set",
: FLAGS_max_num_replicas);
: return false;
: }
> It seems more natural to check that --default_num_replicas for evenness, ra
Yep, that makes sense: I added the non-evenness check for --default_num_replicas as well. I think it's worth keeping the non-evenness check for min/max: otherwise it would be necessary to update error messages to suggest correct replication factor if case of a violation of the tmin/max constraint while creating a table.
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@497
PS3, Line 497: should
> +1
This makes sense, indeed. I updated the code accordingly.
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 16 Jul 2021 04:49:10 +0000
Gerrit-HasComments: Yes
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Attila Bukor (Code Review)" <ge...@cloudera.org>.
Attila Bukor has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 16 Jul 2021 12:34:45 +0000
Gerrit-HasComments: No
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
[catalog_manager] introduce --min_num_replicas flag
This patch introduces a flag for kudu-master to enforce the minimum
number of replicas for newly created tables in a cluster. Unless
overridden, --min_num_replicas is set to 1 by default.
For example, setting --min_num_replicas=3 enforces every new table to
have at least 3 replicas for each of its tablets, so there cannot be
a data loss when a single tablet server in the cluster fails
irrecoverably.
I also added validators for the related flags to keep things more
consistent and report misconfiguration as early as possible.
This patch contains a few test scenarios to cover the newly introduced
functionality.
Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Reviewed-on: http://gerrit.cloudera.org:8080/17684
Tested-by: Alexey Serbin <as...@cloudera.com>
Reviewed-by: Mahesh Reddy <mr...@cloudera.com>
Reviewed-by: Attila Bukor <ab...@apache.org>
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
3 files changed, 228 insertions(+), 59 deletions(-)
Approvals:
Alexey Serbin: Verified
Mahesh Reddy: Looks good to me, but someone else must approve
Attila Bukor: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 5
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Mahesh Reddy (Code Review)" <ge...@cloudera.org>.
Mahesh Reddy has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 4
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Mahesh Reddy <mr...@cloudera.com>
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 16 Jul 2021 07:24:51 +0000
Gerrit-HasComments: No
[kudu-CR] [catalog manager] introduce --min num replicas flag
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17684 )
Change subject: [catalog_manager] introduce --min_num_replicas flag
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc@5356
PS3, Line 5356: const vector<pair<int, string>> cases = {
> can't this be expanded with the new test cases instead of creating separate
Not quite cleanly: it's necessary to have more tablet servers than 1 to properly test min_num_replicas, so it would require resetting the 'cluster_' created by ClientTest::SetUp()
But I guess it's better to do the opposite: move these scenario over into the new test. I'll do so.
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/client/client-test.cc@5359
PS3, Line 5359: {2, "illegal replication factor 2: replication factor must be odd"},
> is there any reason for changing parentheses for colons?
A few reasons: (1) it's less symbols with a semicolon (2) it's inline with the majority of error messages elsewhere (3) it's quite uncommon to put essential information in parenthesis
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@487
PS3, Line 487: if (FLAGS_max_num_replicas % 2 == 0 &&
> is this check duplicated?
I don't see a duplicate: at line 479 the check is against FLAGS_min_num_replicas
http://gerrit.cloudera.org:8080/#/c/17684/3/src/kudu/master/catalog_manager.cc@497
PS3, Line 497: should
> nit: here and below, "should" should be replaced with "must"
The following approach is used here: "must" is when it have to be so unless kudu-master won't start because the settings are inconsistent so a new table could not be created. Using "should" here means "it's not what expected, but kudu-master starts anyway with a warning" -- it's still possible to create a table if overriding the default replication factor explicitly in client API.
Do you rather think the discrepancy between --default_num_replicas and --min_num_replicas should also be a reason to not starting a kudu-master process?
--
To view, visit http://gerrit.cloudera.org:8080/17684
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I86191fcdc1b4ed6670f33ba7176d28dbd1df541f
Gerrit-Change-Number: 17684
Gerrit-PatchSet: 3
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Attila Bukor <ab...@apache.org>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Thu, 15 Jul 2021 14:35:28 +0000
Gerrit-HasComments: Yes