You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/11/30 23:13:43 UTC

[kudu-CR] Reject CREATE TABLE with negative or too-high replication factors 

Hello Dan Burkert, Matthew Jacobs, Will Berkeley,

I'd like you to do a code review.  Please visit

    http://gerrit.cloudera.org:8080/5289

to review the following change.

Change subject: Reject CREATE TABLE with negative or too-high replication factors
......................................................................

Reject CREATE TABLE with negative or too-high replication factors

This adds more master-side checking of the replication factor passed
from the client:

* must be non-negative. Previously passing a negative replication factor
  would result in infinite retries trying to assign replicas.

* put an upper bound of 7, configurable with a new unsafe flag. We
  haven't tested higher replication factors at all, and a
  well-intentioned user may try to set replication factor to a very high
  number to get a table replicated onto every server in their cluster.
  This is common practice in MPP RDBMS, but since we haven't tested it,
  we shouldn't allow it without making the user turn on
  --unlock_unsafe_flags.

Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/experiments/CMakeLists.txt
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
7 files changed, 54 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/5289/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5289/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 3764: TEST_F(ClientTest, TestCreateTableWithBadNumReplicas) {
Would also be nice to test that, after changing the guard rail flag, we _can_ exceed the max.

I guess this feedback also applies to your other guard rail patches.


http://gerrit.cloudera.org:8080/#/c/5289/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 592:   if (data_->num_replicas_ != boost::none) {
Nothing wrong with using boost::optional here, but does this change fix a bug? I thought maybe this fixed the "negative replication factor" issue client-side, but if that were the case, num_replicas would not be greater than or equal to 1 and we wouldn't copy it to the CreateTableRequestPB. What am I missing?


-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.

Change subject: KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5289/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:

Line 3764: TEST_F(ClientTest, TestCreateTableWithBadNumReplicas) {
> Would also be nice to test that, after changing the guard rail flag, we _ca
I don't really see that giving us any incremental coverage, because we already test the "less than the maximum" case in every other test case where we are creating tables. Just seems to add maintenance burden of more test code.


http://gerrit.cloudera.org:8080/#/c/5289/2/src/kudu/client/client.cc
File src/kudu/client/client.cc:

Line 592:   if (data_->num_replicas_ != boost::none) {
> Nothing wrong with using boost::optional here, but does this change fix a b
yea, previously the client side would ignore us if we tried to set a negative replication factor, so even though we had a server-side bug, there wasn't a way to trigger it. Rather than depend on the client to do validation it's better to do the validation server side. Doing it in both places means it's harder to test the server side validation.

Plus, it's better to get an error message than just ignoring them if they set an invalid replication factor.


-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: Yes

[kudu-CR] KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
......................................................................


Patch Set 2: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Matthew Jacobs, Adar Dembo, Will Berkeley, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5289

to look at the new patch set (#3).

Change subject: KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
......................................................................

KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

This adds more master-side checking of the replication factor passed
from the client:

* must be non-negative. Previously passing a negative replication factor
  would result in infinite retries trying to assign replicas.

* put an upper bound of 7, configurable with a new unsafe flag. We
  haven't tested higher replication factors at all, and a
  well-intentioned user may try to set replication factor to a very high
  number to get a table replicated onto every server in their cluster.
  This is common practice in MPP RDBMS, but since we haven't tested it,
  we shouldn't allow it without making the user turn on
  --unlock_unsafe_flags.

Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
6 files changed, 52 insertions(+), 35 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/5289/3
-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Hello Dan Burkert, Matthew Jacobs, Will Berkeley, Kudu Jenkins,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/5289

to look at the new patch set (#2).

Change subject: KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
......................................................................

KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

This adds more master-side checking of the replication factor passed
from the client:

* must be non-negative. Previously passing a negative replication factor
  would result in infinite retries trying to assign replicas.

* put an upper bound of 7, configurable with a new unsafe flag. We
  haven't tested higher replication factors at all, and a
  well-intentioned user may try to set replication factor to a very high
  number to get a table replicated onto every server in their cluster.
  This is common practice in MPP RDBMS, but since we haven't tested it,
  we shouldn't allow it without making the user turn on
  --unlock_unsafe_flags.

Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
6 files changed, 51 insertions(+), 34 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/89/5289/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>

[kudu-CR] KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.

Change subject: KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
......................................................................


Patch Set 3: Code-Review+2

-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-HasComments: No

[kudu-CR] KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged.

Change subject: KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors
......................................................................


KUDU-1775 (part 1). Reject CREATE TABLE with negative or too-high replication factors

This adds more master-side checking of the replication factor passed
from the client:

* must be non-negative. Previously passing a negative replication factor
  would result in infinite retries trying to assign replicas.

* put an upper bound of 7, configurable with a new unsafe flag. We
  haven't tested higher replication factors at all, and a
  well-intentioned user may try to set replication factor to a very high
  number to get a table replicated onto every server in their cluster.
  This is common practice in MPP RDBMS, but since we haven't tested it,
  we shouldn't allow it without making the user turn on
  --unlock_unsafe_flags.

Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Reviewed-on: http://gerrit.cloudera.org:8080/5289
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-test.cc
M src/kudu/client/client.cc
M src/kudu/client/table_creator-internal.cc
M src/kudu/client/table_creator-internal.h
M src/kudu/master/catalog_manager.cc
M src/kudu/master/master.proto
6 files changed, 52 insertions(+), 35 deletions(-)

Approvals:
  Adar Dembo: Looks good to me, approved
  Kudu Jenkins: Verified



-- 
To view, visit http://gerrit.cloudera.org:8080/5289
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I83fedc7cb9722c049c20eb7766605fbb2f966347
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>