You are viewing a plain text version of this content. The canonical link for it is here.

Posted to reviews@kudu.apache.org by "XiaokaiWang (Code Review)" <ge...@cloudera.org> on 2019/07/03 12:18:37 UTC

[kudu-CR] KUDU-2722 (table level): Support table extraConfig 'write enabled' conf

XiaokaiWang has uploaded this change for review. ( http://gerrit.cloudera.org:8080/13796


Change subject: KUDU-2722 (table level): Support table extraConfig 'write_enabled' conf
......................................................................

KUDU-2722 (table level): Support table extraConfig 'write_enabled' conf

Change-Id: Ibd3768eda36d9574be9c41e7cd3dd81cc5ae8f3a
---
M src/kudu/client/client-test.cc
M src/kudu/common/common.proto
M src/kudu/common/wire_protocol.cc
M src/kudu/tablet/tablet_metadata-test.cc
M src/kudu/tools/kudu-admin-test.cc
M src/kudu/tserver/tablet_service.cc
M src/kudu/tserver/ts_tablet_manager-test.cc
M src/kudu/tserver/tserver.proto
8 files changed, 136 insertions(+), 0 deletions(-)



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibd3768eda36d9574be9c41e7cd3dd81cc5ae8f3a
Gerrit-Change-Number: 13796
Gerrit-PatchSet: 1
Gerrit-Owner: XiaokaiWang <xi...@live.com>

[kudu-CR] KUDU-2722 (table level): Support table extraConfig 'write enabled' conf

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13796 )

Change subject: KUDU-2722 (table level): Support table extraConfig 'write_enabled' conf
......................................................................


Patch Set 1:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/13796/1//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/13796/1//COMMIT_MSG@7
PS1, Line 7: KUDU-2722 (table level): Support table extraConfig 'write_enabled' conf
Nit: rewrite as "KUDU-2722 (table level): Support new 'write_enabled' table extra config"


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

http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/client/client-test.cc@2411
PS1, Line 2411: Setted
Set


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/client/client-test.cc@2413
PS1, Line 2413:   ASSERT_FALSE(session->HasPendingOperations());
Not necessary; this is well-tested in other tests.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/client/client-test.cc@2415
PS1, Line 2415:   session->SetTimeoutMillis(10000);
Is this strictly necessary for the test to pass?


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/client/client-test.cc@2418
PS1, Line 2418:   // 1. Test default write enabled is true and will write successfully.
              :   {
              :     unique_ptr<KuduInsert> insert(client_table_->NewInsert());
              :     ASSERT_OK(insert->mutable_row()->SetInt32("key", 11));
              :     ASSERT_OK(insert->mutable_row()->SetInt32("int_val", 54321));
              :     ASSERT_OK(insert->mutable_row()->SetStringCopy("string_val", "hello world"));
              :     ASSERT_OK(session->Apply(insert.release()));
              :     Status s = session->Flush();
              :     ASSERT_TRUE(s.ok()) << s.ToString();
              :   }
Not needed; this is well-tested by virtually every test that tries to write to a tablet.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/client/client-test.cc@2432
PS1, Line 2432:     // Setting Write_enabled is false.
This comment isn't necessary; it essentially duplicates the information on L2429-2430.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/client/client-test.cc@2453
PS1, Line 2453:   // 3. Test reset write enabled is ture, will write successfully.
"Test reenabling writes on the table; now writes should succeed"


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/client/client-test.cc@3956
PS1, Line 3956:   // 1. Alter history max age second to 3600.
Can you update this comment and the one on L3971? No doubt other people will extend this in the future for their new extra config properties, so maybe it's best to rewrite the comments to be generic rather than refer to a particular config by name.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/common/wire_protocol.cc
File src/kudu/common/wire_protocol.cc:

http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/common/wire_protocol.cc@680
PS1, Line 680:         if (!value.empty()) {
Nit: indentation is off in this section.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/common/wire_protocol.cc@684
PS1, Line 684: if (value == "false") {
             :             result.set_write_enabled(false);
             :           }
This should be else if. And if we don't find either of these two, perhaps we should return an "unable to parse" error as in L675.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/tserver/tablet_service.cc@1085
PS1, Line 1085: && 
Nit: move this to the previous line.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/tserver/tablet_service.cc@1086
PS1, Line 1086:      return tablet->metadata()->extra_config()->write_enabled();
Nit: this is the third time you've referenced "tablet->metadata()->extra_config(); consider pulling it out into a local variable for brevity.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/tserver/tablet_service.cc@1147
PS1, Line 1147:                          Status::NotAuthorized("Rejecting Write request: tablet is not writable"),
Are you sure NotAuthorized is the appropriate error here? We currently use it exclusively for authentication/authorization failures; this isn't an instance of that.

Also, as a convention we lower-case the beginning of a status message since we so often prepend other messages to them as the message percolates up the stack.


http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/tserver/tserver.proto
File src/kudu/tserver/tserver.proto:

http://gerrit.cloudera.org:8080/#/c/13796/1/src/kudu/tserver/tserver.proto@107
PS1, Line 107:     // Write not allowed, tablet is downgraded.
Nit: maybe rephrase as "The tablet may not receive new writes"?



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd3768eda36d9574be9c41e7cd3dd81cc5ae8f3a
Gerrit-Change-Number: 13796
Gerrit-PatchSet: 1
Gerrit-Owner: XiaokaiWang <xi...@live.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 04:12:38 +0000
Gerrit-HasComments: Yes

[kudu-CR] KUDU-2722 (table level): Support new 'write enabled' table extra config

Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.

Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13796 )

Change subject: KUDU-2722 (table level): Support new 'write_enabled' table extra config
......................................................................


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/13796/2/src/kudu/tserver/tablet_service.cc
File src/kudu/tserver/tablet_service.cc:

http://gerrit.cloudera.org:8080/#/c/13796/2/src/kudu/tserver/tablet_service.cc@1083
PS2, Line 1083: bool TabletIsWritable(shared_ptr<Tablet> tablet) {
> warning: the parameter 'tablet' is copied for each invocation but only used
This is also a good suggestion that you should adopt.


http://gerrit.cloudera.org:8080/#/c/13796/2/src/kudu/tserver/tablet_service.cc@1084
PS2, Line 1084:   boost::optional<TableExtraConfigPB> extra_config = tablet->metadata()->extra_config();
> This makes a copy, so consider storing a cref:
Looks like you missed this.



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

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibd3768eda36d9574be9c41e7cd3dd81cc5ae8f3a
Gerrit-Change-Number: 13796
Gerrit-PatchSet: 4
Gerrit-Owner: XiaokaiWang <xi...@live.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Grant Henke <gr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: XiaokaiWang <xi...@live.com>
Gerrit-Reviewer: Yao Xu <oc...@gmail.com>
Gerrit-Comment-Date: Wed, 10 Jul 2019 16:21:16 +0000
Gerrit-HasComments: Yes