Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/17917 )
Change subject: KUDU-3326 [delete]: Add soft delete table supports ...................................................................... Patch Set 59: Code-Review+1 (41 comments) Thanks a lot for iteration on this patch! Really appreciated. I apologize for the delay in review. I took another look. It seems this patch is getting closer to be committed soon :) There are few items left, and several nits. Generally, it would be nice to make sure the HMS-related integration would not be broken once this feature is in -- please see the corresponding comment in catalog_manager.cc. Also, it would be nice to converge on the terminology -- I suggest to drop 'trash' and 'trash state' wording everywhere in the code and documentation, switching to 'soft-deleted' instead. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client-internal.h File src/kudu/client/client-internal.h: http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client-internal.h@89 PS59, Line 89: static const uint32_t kTrashTableReservedSeconds = 60 * 60 * 24 * 7; style nit: in each visibility section, statics should come before all other non-static methods/constructors of the section Please move this to be right after the 'pubilc:' tag before the constructor. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client-internal.h@89 PS59, Line 89: kTrashTableReservedSeconds nit: rename into kSoftDeletedTableReservationSeconds ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client-internal.h@125 PS59, Line 125: bool force_on_trashed_table = false Can we get rid of this parameter and rely on 'soft_delete_reservation_seconds' to be 0 if we want to purge the table immediately? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@713 PS59, Line 713: client->SoftDeleteTable(table_name, false, 600); With the current signature of this method, what's the expected behavior of the following call: client->SoftDeleteTable(table_name, true, 600); How long is the reservation period to recall the table after this? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@725 PS59, Line 725: bool force_on_trashed_table = false, With the comment above (L713), does it make sense to drop the 'force_on_thrashed_table' parameter and rely only on the value of the 'reserve_seconds' parameter to control the behavior? Can we have 'reserve_seconds=0' be the equivalent of setting 'force_on_trashed_table=true'? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@808 PS59, Line 808: trash soft-deleted http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@808 PS59, Line 808: only those names pass a substring : /// match on @c filter ... with names matching the specified filter ... http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@816 PS59, Line 816: ListTrashTables nit: maybe, rename this into ListSoftDeletedTables ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.cc File src/kudu/client/client.cc: http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.cc@523 PS59, Line 523: KuduClient:: nit: this extra prefix might be removed, no? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.cc@529 PS59, Line 529: KuduClient:: nit: this prefix might be removed, no? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@146 PS59, Line 146: is_trashed In attempt to stabilize the terminology and use as few names for the same entity as possible, rename this into 'is_soft_deleted'? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@259 PS59, Line 259: is_trashed nit: in attempt to stabilize the terminology and use as few names for the same entity, do you think it make sense to rename this into 'is_soft_deleted'? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@644 PS59, Line 644: kTrashTableType nit: kSoftDeletedTableType ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@669 PS59, Line 669: Soft delete on mark the table with trash state how about: Mark the table as deleted with ability to restore it back within the soft-delete reservation period. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@905 PS59, Line 905: Check whether the table is trashed and outdated how about: Check whether the reservation period for a soft-deleted table has expired. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@906 PS59, Line 906: IsOutdatedTable nit: IsTableExpired ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@909 PS59, Line 909: TableInfoMapType map_type = kAllTableType style nit: per code convention used in Kudu, all input parameters should come before output parameters (https://google.github.io/styleguide/cppguide.html#Inputs_and_Outputs) http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@912 PS59, Line 912: MoveToTrashContainer nit: MoveToSoftDeletedContainer http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@937 PS59, Line 937: call nit: calls http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@957 PS59, Line 957: reserved time nit: reservation period http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@957 PS59, Line 957: trash soft-deleted http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@958 PS59, Line 958: the table, nit: drop this ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@1169 PS59, Line 1169: const std::vector<AlterTableRequestPB::Step>& steps, : Schema* new_schema, : nit: fix the indent (make it align with the very first parameter) http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@1288 PS59, Line 1288: trash_table_names_map_ nit: rename into 'soft_deleted_table_names_map_'? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@596 PS59, Line 596: Add the tablet to the trash map (if the table in trash state) what about: If the table is soft-deleted, add it into the soft-deleted map. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@604 PS59, Line 604: trash soft-deleted http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@1569 PS59, Line 1569: trash_table_names_map_ soft_deleted_table_names_map_ http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2408 PS59, Line 2408: // TODO(kedeng) : trash state need sync to HMS Do you think that's is addressed to the full extent in this patch? If not, that looks like a blocker before the logic of the HMS sync is updated to accommodate for the presence of soft-deleted tables. I guess it's possible to add simple code to disable the soft-delete support if HMS sync is enabled (say, return Status::NotSupported() in such case in a few places). The HMS-related control paths could be addressed in a separate follow-up patch later on. Meanwhile, all those HMS-related changes you already did in this patch might be moved into the follow-up patch, and the logic of this patch would become simpler. What do you think? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2699 PS59, Line 2699: is_trashed_table nit: maybe, rename into 'is_soft_deleted_table' ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2700 PS59, Line 2700: is_outdated_table nit: maybe, rename into 'is_expired_table' ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2702 PS59, Line 2702: (! Is this a typo? I.e., should the negation come before the parenthesis http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2704 PS59, Line 2704: trash soft-delete http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@3098 PS59, Line 3098: is_trashed_table nit: maybe, rename into 'is_soft_deleted_table' ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@3099 PS59, Line 3099: is_outdated_table nit: maybe, rename into 'is_expired_table' ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@6626 PS59, Line 6626: manager the system catalog http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto File src/kudu/master/master.proto: http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto@230 PS59, Line 230: bewteen between http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto@230 PS59, Line 230: reserve seconds reservation time interval (in seconds) http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto@231 PS59, Line 231: trash_state_reserve_seconds Maybe, rename this into 'soft_delete_reservation_seconds' or 'soft_delete_reserved_seconds' ? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto@591 PS59, Line 591: // Force to delete a trashed table. : optional bool force_on_trashed_table = 4 [default = false]; Do we still need this field? I'd expect that once the semantics of 'reserve_seconds=0 is immediate delete' had been added, this field was no longer needed? http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto@603 PS59, Line 603: field is set nit: please document the expected behavior when the field isn't set http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto@624 PS59, Line 624: show_trash Maybe, rename this into 'show_soft_deleted'? -- 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: 59 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Fri, 10 Jun 2022 20:18:56 +0000 Gerrit-HasComments: Yes
