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 8: (25 comments) Would you be open to implementing the C++ client side of this when you're done? 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: throws Exception { Do you actually need the 'throws'? 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. http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/GetTableSchemaResponse.java@102 PS8, Line 102: public TableExtraConfig getExtraConfig() { Doc. 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. http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java File java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java: http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@32 PS8, Line 32: * A table's extra-config. Used to specify the configuration of the table level. Nit: reword as "Extra configuration properties that are customizable on a per-table basis." http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@36 PS8, Line 36: public class TableExtraConfig { Nit: "private static final" instead of "private final static": ~/Source/kudu/java$ git grep "private static final" | wc -l 266 ~/Source/kudu/java$ git grep "private final static" | wc -l 0 http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@39 PS8, Line 39: private final static Set<String> supportedKeys = new HashSet<>(Arrays.asList( I don't think the client should be in the business of deciding which keys are supported and which aren't. For one, it means an old client can never be used to read newer extra-config entries from the server. For two, on CreateTable/AlterTable, the server is going to validate these anyway, so the server should get to decide what's OK and what's not. http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@40 PS8, Line 40: TABLE_HISTORY_MAX_AGE_SEC This set should be empty in this patch, right? http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@111 PS8, Line 111: * @return optionally the value This feels unintuitive to me: if I ask for a key that doesn't exist, I would expect an exception to be thrown, not for the returned value to be optionally empty. http://gerrit.cloudera.org:8080/#/c/12468/8/java/kudu-client/src/main/java/org/apache/kudu/client/TableExtraConfig.java@132 PS8, Line 132: private <T> Optional<T> get(String key, Function<? super String, Optional<T>> parser) { The parsing logic here feels excessive to me. Can we start with simple string->string for now and add parsing if the need arises? 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.*; Could you revert this change? We generally prefer expanded imports to wildcard ones. Same in a few other files. 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: // Tablet history max age time(s), all tablets are same in table. Nit: reword as "Number of seconds to retain history for tablets in this table, including history required to perform diff scans and incremental backups. Reads initiated at a snapshot that is older than this age will be rejected. Equivalent to --tablet_history_max_age_sec". BTW, we should also support a value of -1 (retain all history), so you may want to make this an int32. Then also doc the meaning of -1 and of 0 (which I presume means "use the value of --tablet_history_max_age_sec"). Also, I thought the goal of this first patch was to establish the extra config mechanism, and only to populate it in a subsequent patch? 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: const TableExtraConfigPB& extra_config = req.extra_config(); Please add a test to master-test.cc where you send a specially crafted CreateTable RPC (and AlterTable too) without the extra config set. 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: optional TableExtraConfigPB extra_config = 8; Why does this need to be persisted in every tablet? Can't we store it just in the table and fetch it from there at runtime when sending to the tablets? http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/master/master.proto@613 PS8, Line 613: optional AlterExtraConfig alter_extra_config = 8; I don't think this needs to be a step; it could be inline in AlterTableRequestPB (a la new_table_name). Though I agree that the contents should be merged into the existing extra config. 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: TableMetadataLock l(table.get(), LockMode::READ); Rather than reacquiring the lock, make a copy of the extra config somewhere between L247-343. Also, use SecureDebugString() here. 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: Status Create(bool first_time, const TableExtraConfigPB& extra_config) { Doesn't seem like any test needs to customize this; could you just create an empty one for the time being and forgo the Create overload? 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(const TableExtraConfigPB& extra_config); Can we pass this by value and std::move it? 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: const TableExtraConfigPB& extra_config, In these functions you should pass extra_config by value and std::move() it. 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's extra config and verifying that the tablet superblocks were updated? http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tablet/transactions/alter_schema_transaction.h@74 PS8, Line 74: const TableExtraConfigPB& new_extra_config() const { If the intent is to mutate the result, you need to return it as TableExtraConfigPB* 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: Status CreateNewTabletWithExtraConfig( There aren't that many callers to CreateNewTablet() in this test; I'd rather you extended it instead of adding an overload. http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tserver/ts_tablet_manager-test.cc@182 PS8, Line 182: TEST_F(TsTabletManagerTest, TestCreateTabletWithExtraConfig) { Could you just modify TestCreateTablet, since so much of the code is identical? 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: extra_config, Pass by value and std::move(). http://gerrit.cloudera.org:8080/#/c/12468/8/src/kudu/tserver/tserver_admin.proto File src/kudu/tserver/tserver_admin.proto: PS8: I think we should represent the extra config as a generic string->string protobuf map on the wire. That way the client doesn't need to know anything about which specific keys are supported; the server can be the source of truth. -- 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: 8 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, 17 May 2019 01:01:27 +0000 Gerrit-HasComments: Yes
