KeDeng 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 6: (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' fla > nit: '--default_deleted_table_reserve_seconds' flag on master. Done 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: /// The deleted table may turn to soft-deleted status with the flag > nit: --default_deleted_table_reserve_seconds Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/client/client.h@747 PS5, Line 747: > How about set reserve_seconds as std::optional<uint32_t>, then with_reserve Done 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: Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2418 PS5, Line 2418: DCHECK(s.ok()); > nit: --default_deleted_table_reserve_seconds Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2421 PS5, Line 2421: // Kudu client with the precise value for 'reserve_seconds', and the field's value from the > nit: --default_deleted_table_reserve_seconds Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2422 PS5, Line 2422: est should be taken as-is re > How about Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2423 PS5, Line 2423: seconds' flag at th > reserve_seconds Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2428 PS5, Line 2428: bool enable_soft_delete_on_master = FLAGS_default_deleted_table_reserve_seconds != 0; > It's not needed to check if 's' is OK or not, has been checked before in li Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433 PS5, Line 2433: // soft-delete has enabled > nit: 'soft deleted' or 'soft-deleted' Done http://gerrit.cloudera.org:8080/#/c/19047/5/src/kudu/master/catalog_manager.cc@2433 PS5, Line 2433: > nit: 'soft deleted again' ? Done 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_int32(reserve_seconds, -1, > I guess it's not necessary if set the --reserve_seconds default value as -1 Done -- 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: 6 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: Mon, 31 Oct 2022 03:13:15 +0000 Gerrit-HasComments: Yes
