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 10: (18 comments) Well, it seems that the failure of jenkins is not caused by this patch. http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java File java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java: http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/AlterTableOptions.java@390 PS8, Line 390: */ > Do you actually need the 'throws'? Remove it. http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java File java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java: http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java@36 PS8, Line 36: > Update this list of params. Done http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java@102 PS8, Line 102: } > Doc. Done http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java File java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java: http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/KuduTable.java@53 PS8, Line 53: > Update. Done http://gerrit.cloudera.org:8080/#/c/12468/8/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/8/java/kudu-client/src/test/java/org/apache/kudu/client/TestAlterTable.java@27 PS8, Line 27: import java.util.ArrayList; > Could you revert this change? We generally prefer expanded imports to wildc Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/common/common.proto File src/kudu/common/common.proto: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/common/common.proto@431 PS8, Line 431: // Number of seconds to retain history for tablets in this table, > Nit: reword as "Number of seconds to retain history for tablets in this tab Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/catalog_manager.cc@1583 PS8, Line 1583: } > Please add a test to master-test.cc where you send a specially crafted Crea By default, there is no extra-config set in CreateTable/AlterTable RPC. So, I think it is working. http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/master.proto@147 PS8, Line 147: // The on-disk entry in the sys.catalog table ("metadata" column) for > Why does this need to be persisted in every tablet? Can't we store it just Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/master.proto@613 PS8, Line 613: // necessary when partitions are being added or dropped. > I don't think this needs to be a step; it could be inline in AlterTableRequ Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/master_path_handlers.cc File src/kudu/master/master_path_handlers.cc: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/master_path_handlers.cc@423 PS8, Line 423: peer_json["is_leader"] = e.first.is_leader; > Rather than reacquiring the lock, make a copy of the extra config somewhere Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/tablet-harness.h File src/kudu/tablet/tablet-harness.h: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/tablet-harness.h@89 PS8, Line 89: if (first_time) { > Doesn't seem like any test needs to customize this; could you just create a Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/tablet_metadata.h File src/kudu/tablet/tablet_metadata.h: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/tablet_metadata.h@138 PS8, Line 138: void SetExtraConfig(TableExtraConfigPB extra_config); > Can we pass this by value and std::move it? Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/tablet_metadata.cc@93 PS8, Line 93: boost::optional<OpId> tombstone_last_logged_opid, > In these functions you should pass extra_config by value and std::move() it Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/transactions/alter_schema_transaction.h File src/kudu/tablet/transactions/alter_schema_transaction.h: PS8: > Not seeing this used anywhere; do we have test coverage of altering a table Done http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/transactions/alter_schema_transaction.h@74 PS8, Line 74: TableExtraConfigPB new_extra_config() const { > If the intent is to mutate the result, you need to return it as TableExtraC I think here is the same like new table name, there is no need to return a pointer. 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@102 PS8, Line 102: > There aren't that many callers to CreateNewTablet() in this test; I'd rathe Done 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 > is identical? I think it's better to create a test case for extra config, because it can cover more code branches. http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tserver/ts_tablet_manager.cc@450 PS8, Line 450: > Pass by value and std::move(). Done -- 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: 10 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, 21 May 2019 09:56:26 +0000 Gerrit-HasComments: Yes
