KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/17917 )
Change subject: KUDU-3326 [delete]: Add soft delete table supports ...................................................................... Patch Set 60: (38 comments) 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: > nit: rename into kSoftDeletedTableReservationSeconds ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client-internal.h@89 PS59, Line 89: ~Data(); > style nit: in each visibility section, statics should come before all other Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client-internal.h@125 PS59, Line 125: bool force_on_soft_deleted_table = > Can we get rid of this parameter and rely on 'soft_delete_reservation_secon The 'force_on_trashed_table' only works for trash state table. If we want to delete the normal table immediately, just make reserve_seconds to 0 is OK. But for soft-deleted table, we need to set 'force_on_trashed_table'. At this point, the soft-deleted table will be deleted immediately with the set of 'force_on_trashed_table' regardless of the 'reserve_seconds'. 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 600 seconds. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@725 PS59, Line 725: bool force_on_soft_deleted_table = f > With the comment above (L713), does it make sense to drop the 'force_on_thr The 'force_on_trashed_table' use to remind users whether to delete the soft-deleted state table. If we want to delete the normal table immediately, just make reserve_seconds to 0. The 'force_on_trashed_table' will not working for normal tables. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@808 PS59, Line 808: soft- > soft-deleted Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@808 PS59, Line 808: tables only those names pass a substring : /// with names matchin > ... with names matching the specified filter ... Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.h@816 PS59, Line 816: ListSoftDeleted > nit: maybe, rename this into ListSoftDeletedTables ? Done 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: deadline = > nit: this extra prefix might be removed, no? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/client/client.cc@529 PS59, Line 529: e deadline = > nit: this prefix might be removed, no? Done 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_soft_de > In attempt to stabilize the terminology and use as few names for the same e Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@259 PS59, Line 259: is_soft_de > nit: in attempt to stabilize the terminology and use as few names for the s Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@644 PS59, Line 644: kSoftDeletedTab > nit: kSoftDeletedTableType ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@669 PS59, Line 669: Mark the table as soft-deleted with ability to > how about: Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@905 PS59, Line 905: tus IsTableExpired(const TableIdentifierPB& tab > how about: Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@906 PS59, Line 906: > nit: IsTableExpired ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@909 PS59, Line 909: > style nit: per code convention used in Kudu, all input parameters should co Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@912 PS59, Line 912: > nit: MoveToSoftDeletedContainer Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@937 PS59, Line 937: Tabl > nit: calls Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@957 PS59, Line 957: > nit: reservation period Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@957 PS59, Line 957: sePB* > soft-deleted Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@958 PS59, Line 958: > nit: drop this ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@1169 PS59, Line 1169: : Status ApplyAlterPartitioningSteps(const TableMetadataLock& l, : > nit: fix the indent (make it align with the very first parameter) Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.h@1288 PS59, Line 1288: tablet_map_; > nit: rename into 'soft_deleted_table_names_map_'? Done 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: If the table is soft-deleted, add it into the soft-deleted ma > what about: Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@604 PS59, Line 604: soft- > soft-deleted Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@1569 PS59, Line 1569: soft_deleted_table_nam > soft_deleted_table_names_map_ Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2408 PS59, Line 2408: > Do you think that's is addressed to the full extent in this patch? If not, In my opinion, this is not blocked. For example, a table in soft delete status in kudu is equal ? normal status in HMS, which is only inaccurate for HMS and does not affect its use. The tables deleted in kudu are the same in HMS, so both sides are consistent. Subsequent patches only need to synchronize the soft deleted status? which will make the display exactly the same on both sides. http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2699 PS59, Line 2699: > nit: maybe, rename into 'is_soft_deleted_table' ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2700 PS59, Line 2700: is_soft_deleted_t > nit: maybe, rename into 'is_expired_table' ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2702 PS59, Line 2702: ab > Is this a typo? I.e., should the negation come before the parenthesis Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@2704 PS59, Line 2704: nd(Su > soft-delete Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@3098 PS59, Line 3098: auto& tablet : n > nit: maybe, rename into 'is_soft_deleted_table' ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@3099 PS59, Line 3099: lets_to_add->empl > nit: maybe, rename into 'is_expired_table' ? Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/catalog_manager.cc@6626 PS59, Line 6626: > the system catalog Done 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@591 PS59, Line 591: // Force to delete a soft-deleted table. : optional bool force_on_soft_deleted_table = 4 [default = fa > Do we still need this field? I'd expect that once the semantics of 'reserv We just set reserve_seconds to 0 if we want to delete the normal table directly. But is not valid for the soft——deleted table. We need to set 'force_on_trashed_table' to 'True' if we want to delete the soft-deleted table immediately. 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 Done http://gerrit.cloudera.org:8080/#/c/17917/59/src/kudu/master/master.proto@624 PS59, Line 624: oft_delete > Maybe, rename this into 'show_soft_deleted'? Done -- 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: 60 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: Thu, 16 Jun 2022 02:21:32 +0000 Gerrit-HasComments: Yes
