Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12468 )
Change subject: KUDU-2514 Part 1: Support extra config for table. ...................................................................... Patch Set 11: (18 comments) http://gerrit.cloudera.org:8080/#/c/12468/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java: http://gerrit.cloudera.org:8080/#/c/12468/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@501 PS11, Line 501: // Check for expected Should also test: 1. That if you change the value from 3600 to something else, the change happens. 2. Once you implement "reset to default", that you can do that too. http://gerrit.cloudera.org:8080/#/c/12468/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java File java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java: http://gerrit.cloudera.org:8080/#/c/12468/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@203 PS11, Line 203: assertFalse(client.getTablesList().getTablesList().isEmpty()); : assertTrue(client.getTablesList().getTablesList().contains(TABLE_NAME)); Don't need this; other tests already do this. http://gerrit.cloudera.org:8080/#/c/12468/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@210 PS11, Line 210: : // Check that we can delete it. : client.deleteTable(TABLE_NAME); : assertFalse(client.getTablesList().getTablesList().contains(TABLE_NAME)); Don't need this; it's not relevant to extra configs. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/common/common.proto@434 PS11, Line 434: // age will be rejected. Equivalent to --tablet_history_max_age_sec Nit: terminate the sentence with a period. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/common/wire_protocol.cc File src/kudu/common/wire_protocol.cc: http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/common/wire_protocol.cc@657 PS11, Line 657: if (strcmp(config.first.c_str(), kTableHistoryMaxAgeSec) == 0) { Could we just do a string comparison? I think it's OK to implicitly convert kTableHistoryMaxAgeSec into a std::string. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/common/wire_protocol.cc@663 PS11, Line 663: else { : return Status::InvalidArgument("Unknown extra configuration properties", config.first); : } For better forward compatibility, let's just ignore entries whose keys we don't recognize. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc@1585 PS11, Line 1585: that Drop 'that' http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc@1586 PS11, Line 1586: map<string, string> extra_configs(req.extra_configs().begin(), req.extra_configs().end()); : TableExtraConfigPB extra_config_pb; : RETURN_NOT_OK(ExtraConfigToPB(extra_configs, &extra_config_pb)); Maybe add wire_protocol methods for ExtraConfigToPBMap and ExtraConfigFromPBMap? Can't use them in AlterTable because of the merging behavior, but they'd simplify CreateTable and GetTableSchema a bit. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc@1757 PS11, Line 1757: const TableExtraConfigPB& extra_config_pb) { You can pass this by value and move it rather than passing by ref and copying it. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc@2567 PS11, Line 2567: RETURN_NOT_OK(ExtraConfigToPB(new_extra_configs, &new_extra_config_pb)); I think we need to provide some way to 'reset' a config back to its default value. The way the code is written, I don't see how to do that. The client can either: 1. Provide a map entry, in which case the entry's value is carried over into the corresponding extra config. 2. Not provide a map entry, in which case the corresponding extra config is left untouched. Maybe we can add to this the ability to: 3. Provide a map entry with an empty string as its value, in which case the corresponding extra config is reset to its default value? There are protobuf methods that should help you accomplish this. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/master_path_handlers.cc@429 PS11, Line 429: string str_extra_configs; : for (const auto& config : extra_configs) { : str_extra_configs += config.first; : str_extra_configs += " : "; : str_extra_configs += config.second; : str_extra_configs += "\n"; : } You might find the methods in gutil/strings/join.{cc.h} useful for this. JoinMapKeysAndValues perhaps? http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet-test-util.h File src/kudu/tablet/tablet-test-util.h: http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet-test-util.h@147 PS11, Line 147: void AlterSchema(const Schema& schema, boost::optional<TableExtraConfigPB> extra_config) { You can give the new arg a default value of boost::none to avoid having to update existing callers. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet_metadata-test.cc File src/kudu/tablet/tablet_metadata-test.cc: http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet_metadata-test.cc@114 PS11, Line 114: TEST_F(TestTabletMetadata, TestLoadFromDisk) { Only having reviewed the new test did I realize that it's mostly a copy of TestLoadFromSuperBlock. Perhaps you could augment that test a bit instead of duplicating so much? http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet_metadata-test.cc@115 PS11, Line 115: // Write some data to the tablet and flush. Do you actually need written data and flushes to test the extra config round trip? http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet_metadata-test.cc@118 PS11, Line 118: writer_->Insert(*row); ASSERT_OK on Insert calls. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet_metadata-test.cc@131 PS11, Line 131: // Shut down the tablet. : harness_->tablet()->Shutdown(); I don't think you need this for the rest of the test to pass. http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet_metadata-test.cc@155 PS11, Line 155: LOG(INFO) << "Superblocks match:\n" : << pb_util::SecureDebugString(superblock_pb_1); You can drop this; the test passing is all the indication we need that the superblocks matched. http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tserver/ts_tablet_manager-test.cc File src/kudu/tserver/ts_tablet_manager-test.cc: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tserver/ts_tablet_manager-test.cc@182 PS8, Line 182: ASSERT_NE(boost::none, replica->tablet()->metadata()->extra_config()); > > Could you just modify TestCreateTablet, since so much of the code This new test is a superset of the old test in terms of coverage, so what value does the old test now add? That said, there is some missing coverage that may justify a new test. I'm not seeing anything that verifies that the extra config in the tablet's superblock has changed following a successful AlterSchema. -- To view, visit http://gerrit.cloudera.org:8080/12468 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0514507dca95602a97e954d1db464b907e073aae Gerrit-Change-Number: 12468 Gerrit-PatchSet: 11 Gerrit-Owner: Yao Xu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Yao Xu <[email protected]> Gerrit-Comment-Date: Fri, 31 May 2019 05:44:30 +0000 Gerrit-HasComments: Yes
