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 <xiaokai.w...@live.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yao Xu <ocla...@gmail.com> Gerrit-Comment-Date: Wed, 10 Jul 2019 04:12:38 +0000 Gerrit-HasComments: Yes
