Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17917 )
Change subject: KUDU-3326 [delete]: Add soft delete table supports ...................................................................... Patch Set 22: (5 comments) Did a high level pass of the server-side code first. I'm wary of relying on AlterTable RPCs here, and think that we should instead make soft deletion be a table state in the system catalog. http://gerrit.cloudera.org:8080/#/c/17917/22/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17917/22/src/kudu/master/catalog_manager.cc@1505 PS22, Line 1505: trash_table_names_map_.clear(); How is this repopulated? I'd expect we need to check the system catalog entries to see if there's some "trashed" field set for our tables. http://gerrit.cloudera.org:8080/#/c/17917/22/src/kudu/master/catalog_manager.cc@6214 PS22, Line 6214: trash_table_names_map_[table_name] = table; What happens if 'trash_table_names_map_' already has a trashed table of this name? Conceptually, it seems fine to "forget" about the table, but what happens to its data? Is the old data ever deleted? http://gerrit.cloudera.org:8080/#/c/17917/22/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: http://gerrit.cloudera.org:8080/#/c/17917/22/src/kudu/master/master_service.cc@581 PS22, Line 581: modify_external_catalogs I don't think this is doing the right thing. If we have requested to modify external catalogs, a delete operation should delete the record in the external catalog, not alter it. http://gerrit.cloudera.org:8080/#/c/17917/22/src/kudu/master/master_service.cc@578 PS22, Line 578: DCHECK(!is_trashed_table); : AlterTableRequestPB alter_req; : alter_req.mutable_table()->CopyFrom(req->table()); : alter_req.set_modify_external_catalogs(req->modify_external_catalogs()); : (*alter_req.mutable_new_extra_configs())[kTableMaintenancePriority] : = std::to_string(-FLAGS_max_priority_range); : (*alter_req.mutable_new_extra_configs())[kTableConfigReserveSeconds] : = std::to_string(req->reserve_seconds()); : int64_t now = static_cast<int64_t>(WallTime_Now()); : (*alter_req.mutable_new_extra_configs())[kTableConfigTrashStateTimestamp] : = std::to_string(now); : : AlterTableResponsePB alter_resp; : Status s = server_->catalog_manager()->AlterTableRpc(alter_req, &alter_resp, rpc, : kWithoutExternalRequest); : if (!s.ok()) { : CheckRespErrorOrSetUnknown(s, &alter_resp); : resp->set_allocated_error(alter_resp.release_error()); : rpc->RespondSuccess(); : return; : } The failure scenarios here are difficult to reason about. What if there's a leadership change in between the alter and the deletion? What if the master is shut down while this is happening? Right now I think we are better off not performing any alter, and leaving the trash operation be a single master-only (and external catalogs) operation, similar to table deletion (that is, by setting the SysTabletsEntryPB::State to DELETED; perhaps we should have a state like SOFT_DELETED or somesuch?). Having this be a single operation also means that there's a single atomic write to the system catalog tablet. At the very least, additional functionality like this should be pushed into a separate patch. http://gerrit.cloudera.org:8080/#/c/17917/22/src/kudu/master/master_service.cc@616 PS22, Line 616: IsOutdatedTable Could we move more of the business logic into the CatalogManager, rather than having it seep into the service code? For example, could we always call RecallDeletedTableRpc(), but have the implementation return an error if the trashed table is outdated? I think it might help make it easier to reason about the code if the constraints are built into the functions in that way (same with MoveToTrashContainer, DeleteTable, etc.). It also lets us do some of the checking under a single lock check, rather than taking the catalog manager lock twice (once in IsOutdatedTable() and once in RecallDeletedTableRpc()). -- To view, visit http://gerrit.cloudera.org:8080/17917 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I3d1dddfbca55a5c4bcac4028157325ad618ea665 Gerrit-Change-Number: 17917 Gerrit-PatchSet: 22 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Tue, 18 Jan 2022 01:13:45 +0000 Gerrit-HasComments: Yes
