Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18864 )
Change subject: KUDU-3326 [master] add flag FLAGS_deleted_table_reserve_seconds to change the cluster-wide behavior of the DeleteTable() RPC. ...................................................................... Patch Set 14: (2 comments) http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2422 PS14, Line 2422: req > Yes, the update op is necessary and I do this at line 2469. Ah, indeed. That's a bit strange that the same condition on FLAGS_deleted_table_reserve_seconds is involved twice, though. Also, is it possible to update this section of the code to have just one call site for SoftDeleteTable(), not two? As of PS14, SoftDeleteTable() has two call sites in this small code section: lines 2422 and 2429. I hope that having just one call site could make the code more readable. http://gerrit.cloudera.org:8080/#/c/18864/14/src/kudu/master/catalog_manager.cc@2421 PS14, Line 2421: if (FLAGS_deleted_table_reserve_seconds > 0) { : return SoftDeleteTable(req, resp, rpc); : } > The GetTableStates(...) function checks the soft-deleted status base the me I'm not sure I understand that explanation. >From the code I can see that nothing prevents sending DeleteTableRequestPB() >for a soft-deleted, but not yet purged table, right? Now, what if >FLAGS_deleted_table_reserve_seconds > 0 for the cluster? From the code I can >see that SoftDeleteTable() would be called again for the soft-deleted table. If you insist that's impossible, could you then please add a test scenario for the following: 1) A Kudu cluster runs with --deleted_table_reserve_seconds=600 2) A table X is created 3) A request to soft-delete table X arrives with reserve_seconds == 60 4) Right after that, a request to purge the soft-deleted table X arrives 5) The result should be: the table X should be immediately purged, so no table X should be in the system after that. Thanks! -- To view, visit http://gerrit.cloudera.org:8080/18864 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7cf3021fa0e9a425d5f8091ea3336c55a3c14bbc Gerrit-Change-Number: 18864 Gerrit-PatchSet: 14 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Thu, 08 Sep 2022 04:40:43 +0000 Gerrit-HasComments: Yes
