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 13: (4 comments) http://gerrit.cloudera.org:8080/#/c/19047/12/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java File java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java: http://gerrit.cloudera.org:8080/#/c/19047/12/java/kudu-client/src/main/java/org/apache/kudu/client/DeleteTableRequest.java@37 PS12, Line 37: private Boolean withReserveSeconds = false; The same to C++ client, is the Boolean variable really needed? http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/19047/13/src/kudu/client/client.h@740 PS13, Line 740: filed nit: field? 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@747 PS5, Line 747: uint32_t reserve_seconds = 0) KUDU_NO_EXPORT; > I'm sorry I failed to change this because this will make python UT fail. What's the failure details? Or we can use flag --soft_table_reserve_time_s in tool_action_table.cc, that is -1 means not specified(use server side config), 0 means purge immediately, larger than 0 means reserve some seconds. After all, it would be better to reduce the count pf parameters, and avoid confict like, set with_reserve_seconds=false but set reserve_seconds=120. http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/19047/12/src/kudu/tools/tool_action_table.cc@202 PS12, Line 202: soft_table_rese > Done I‘d prefer the original 'reserve_seconds', it’s corresponding to the server side flag 'default_deleted_table_reserve_seconds', here the flag is only used in 'kudu table delete' command, so something like 'deleted, table' can be omitted. "soft table" is a bit of strange... -- 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: 13 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Sun, 06 Nov 2022 13:33:13 +0000 Gerrit-HasComments: Yes
