Yao Xu 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 16: (15 comments) Thanks for the comments. I made some changes according to the 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: table = client.openTable(tableName); > Should also test: Done 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: KuduTable table = client.openTable(TABLE_NAME); : extraConfigs = table.getExtraConfig(); > Don't need this; other tests already do this. Done http://gerrit.cloudera.org:8080/#/c/12468/11/java/kudu-client/src/test/java/org/apache/kudu/client/TestKuduClient.java@210 PS11, Line 210: * Test the scanner behavior when a scanner is used beyond : * the scanner ttl without calling keepAlive. : */ : @Test(timeout = 100000) > Don't need this; it's not relevant to extra configs. Done 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. Done 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: map<string, string>* configs) { > Could we just do a string comparison? I think it's OK to implicitly convert Done http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/common/wire_protocol.cc@663 PS11, Line 663: : : Statu > For better forward compatibility, let's just ignore entries whose keys we d Done 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: _OK( > Drop 'that' Done http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc@1586 PS11, Line 1586: : scoped_refptr<TableInfo> table; : { > Maybe add wire_protocol methods for ExtraConfigToPBMap and ExtraConfigFromP Done http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc@1757 PS11, Line 1757: table->mutable_metadata()->StartMutation(); > You can pass this by value and move it rather than passing by ref and copyi Done http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/master/catalog_manager.cc@2567 PS11, Line 2567: // Set to true if there are schema changes, or the table is renamed. > 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. Agree. I originally wanted to provide another API to 'reset'. However, it seems that your solution is better. Thank you very much. :) 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; : JoinMapKeysAndValues(extra_configs, " : ", "\n", &str_extra_configs); : (*output)["extra_config"] = str_extra_configs; : : EasyJson summary_json = output->Set("tablets_summary", EasyJson::kArray); : for (const auto& entry : summary_states) { : > You might find the methods in gutil/strings/join.{cc.h} useful for this. Jo Done 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, > You can give the new arg a default value of boost::none to avoid having to Done 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: ASSERT_OK(meta->ReplaceSuperBlock(superblock_pb_1)); > Only having reviewed the new test did I realize that it's mostly a copy of Done http://gerrit.cloudera.org:8080/#/c/12468/11/src/kudu/tablet/tablet_metadata-test.cc@155 PS11, Line 155: ASSERT_GT(initial_size, 0); : > You can drop this; the test passing is all the indication we need that the Done 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_TRUE(tablet_manager_->LookupTablet(tablet2, &replica2)); > This new test is a superset of the old test in terms of coverage, so what v Done. The test for AlterSchema can be found in TestTabletMetadata.TestLoadFromSuperBlock. :) -- 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: 16 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: Tue, 04 Jun 2019 12:40:09 +0000 Gerrit-HasComments: Yes
