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 17: (8 comments) Overall looks good, just a few nits. I also added a generic comment on how to make the semantics of DeleteTable RPC a bit clearer. http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5221 PS17, Line 5221: ASSERT_FALSE(FLAGS_deleted_table_reserve_seconds); nit for here and below: please replace with ASSERT_EQ(0, FLAGS_deleted_table_reserve_seconds) for easier mapping into the type of the --deleted_table_reserve_seconds flag http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5222 PS17, Line 5222: = 1; To prevent flakiness in case of scheduler anomalies and/or slow inferior test VM, maybe set this to something greater than 1 (e.g. 60)? http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5242 PS17, Line 5242: ASSERT_EQ(0, tables.size()); nit: if using ASSERT_TRUE(tables.empty()) at line 5251, maybe use it here as well? http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5258 PS17, Line 5258: ASSERT_FALSE(FLAGS_deleted_table_reserve_seconds); nit: please replace with ASSERT_EQ(0, FLAGS_deleted_table_reserve_seconds) for easier mapping into the type of the --deleted_table_reserve_seconds flag http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5261 PS17, Line 5261: // Delete the table, the table won't be purged immediately if : // FLAGS_deleted_table_reserve_seconds set to anything greater than 0. Is this relevant if non-zero 'reserve_seconds' comes with DeleteTableRequestRPC? http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/client/client-test.cc@5279 PS17, Line 5279: ASSERT_EQ(0, tables.size()); nit: if using ASSERT_TRUE(tables.empty()) at line 5288, maybe use it here as well? http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc@413 PS17, Line 413: nit: remove the extra space http://gerrit.cloudera.org:8080/#/c/18864/17/src/kudu/master/catalog_manager.cc@2419 PS17, Line 2419: // Reserve seconds equal 0 means delete it directly. : if (req.reserve_seconds() == 0 && : (FLAGS_deleted_table_reserve_seconds == 0 || is_soft_deleted_table)) { : return DeleteTableRpc(req, resp, rpc); : } One comment which is a bit off-topic (or just a broader note). It's not necessary to address right in this patch, just curious what you think of this. Having a separate patch to address this is a better option, IMO. After some consideration, I noticed there is room for improvement on interpreting the 'reserve_seconds' field. That's to make the semantics of DeleteTable clearer and have the behavior to be backward-compatible for older clients that are not aware of the 'reserve_seconds' field in DeleteTableRequestPB. >From what I can see, that requires only cosmetic changes here (and maybe >similar cosmetic changes in the client as well). What if instead of comparing the 'reserve_seconds' with 0, the server side first checked whether the field is set at all in DeleteTableRequestPB (i.e. called DeleteTableRequestPB::has_reserve_seconds())? If the field is not specified by the client in DeleteTableRequestPB, then we know that's a request either: * from a legacy client to drop the table immediately * from a new client to drop the table immediately In both cases the server side takes the liberty to interpret the request of immediately dropping the table per current setting of its --deleted_table_reserve_seconds flag. If the field is specified by the client, then we know that's a request coming from the a newer Kudu client with the precise value for 'reserve_seconds', and the field's value from the request should be taken as-is regardless of the current setting of the --deleted_table_reserve_seconds flag at the server side. With that, the semantics of interpreting DeleteTableRequestPB at the server side becomes a bit clearer, it seems. For regular a table: if 'reserve_seconds' field specified in DeleteTableRequestPB then soft-delete the table with the grace period of 'reserve_seconds' (if that's 0, then the table will be deemed expired and purged as soon as possible by the ExpiredReservedTablesDeleterThread thread) else if FLAGS_deleted_table_reserve_seconds != 0 then soft-delete the table with the grace period of FLAGS_deleted_table_reserve_seconds else purge the table immediately endif endif For a soft-deleted table: if 'reserve_seconds' field specified in DeleteTableRequestPB then if 'reserve_seconds' == 0 then immediately purge the table else report Status::IllegalState("table is already soft-deleted") endif else report Status::NotFound("table not found") endif What do you think? -- 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: 17 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, 15 Sep 2022 02:48:13 +0000 Gerrit-HasComments: Yes
