[kudu-CR] KUDU-3326 [master] add improvement on interpreting the 'reserve seconds' field for DeleteRPC

["Yingchun Lai (Code Review)" <ge...@cloudera.org>]

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 3:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:

http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2412
PS3, Line 2412:   Status s = GetTableStates(req.table(), kAllTableType, &is_soft_deleted_table, &is_expired_table);
The error status should be dealt at first? Now the only error is NotFound.


http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2414
PS3, Line 2414: the 
nit: remove it


http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/catalog_manager.cc@2418
PS3, Line 2418:   if (req.has_reserve_seconds()) {
Maybe these two cases (whether use specified 'reserve_seconds' or not) can be unified to simlify code, since there are almost duplicate code?


http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto
File src/kudu/master/master.proto:

http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto@591
PS3, Line 591: 'default_deleted_table_reserve_seconds'
nit: it's a habit to add two minus chars before the flag, i.e. '--default_deleted_table_reserve_seconds', to expose that it's a flag.


http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/master/master.proto@593
PS3, Line 593: default_deleted_table_reserve_seconds
nit: same


http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

http://gerrit.cloudera.org:8080/#/c/19047/3/src/kudu/tools/tool_action_table.cc@201
PS3, Line 201: DEFINE_bool(with_reserve_seconds, false,
If '--reserve_seconds' is not 0, that implicit says that it is specified and we should use it, so is it duplicate to add such a '--with_reserve_seconds' flag?



-- 
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: 3
Gerrit-Owner: KeDeng <kd...@gmail.com>
Gerrit-Reviewer: Alexey Serbin <al...@apache.org>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Yingchun Lai <ac...@gmail.com>
Gerrit-Comment-Date: Sat, 15 Oct 2022 13:29:58 +0000
Gerrit-HasComments: Yes