Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19047 )
Change subject: KUDU-3326 [master] add improvement on interpreting the 'reserve_seconds' field for DeleteRPC ...................................................................... Patch Set 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/19047/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java: http://gerrit.cloudera.org:8080/#/c/19047/5/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java@719 PS5, Line 719: 'default_deleted_table_reserve_seconds' flag. nit: '--default_deleted_table_reserve_seconds' flag on master. http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@687 PS5, Line 687: /// default_deleted_table_reserve_seconds set to nonzero on the master side. nit: --default_deleted_table_reserve_seconds http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@747 PS5, Line 747: uint32_t reserve_seconds = 0) KUDU_NO_EXPORT; How about set reserve_seconds as std::optional<uint32_t>, then with_reserve_seconds is not needed. http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2413 PS5, Line 2413: The only error is NotFound How about adjust the code like: // The only error is NotFound, ... if (s.IsNotFound()) { return DeleteTableRpc(req, resp, rpc); } DCHECK(s.ok()); ... http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2418 PS5, Line 2418: // 'default_deleted_table_reserve_seconds' flag at the server side. nit: --default_deleted_table_reserve_seconds http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2421 PS5, Line 2421: // controlled by the 'default_deleted_table_reserve_seconds' flag. nit: --default_deleted_table_reserve_seconds http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2422 PS5, Line 2422: enable_soft_delete_on_client How about bool enable_soft_delete_on_client = req.has_reserve_seconds() && req.reserve_seconds() != 0; http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2423 PS5, Line 2423: has_reserve_seconds reserve_seconds http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2428 PS5, Line 2428: if (s.ok() && is_soft_deleted_table && It's not needed to check if 's' is OK or not, has been checked before in line 2414. http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433 PS5, Line 2433: deleted nit: 'soft deleted again' ? http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433 PS5, Line 2433: Status::InvalidArgument(Substitute("soft_deleted table $0 should not be deleted", nit: 'soft deleted' or 'soft-deleted' http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/tools/tool_action_table.cc@201 PS5, Line 201: DEFINE_bool(with_reserve_seconds, false, I guess it's not necessary if set the --reserve_seconds default value as -1. Then 0 means not reserve, and larger than 0 means reserve with some seconds? -- To view, visit http://gerrit.cloudera.org:8080/19047 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I604caf5d7326e27acd6d626b482376fc6eed1b0f Gerrit-Change-Number: 19047 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Sun, 30 Oct 2022 11:42:10 +0000 Gerrit-HasComments: Yes
