KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/20178 )
Change subject: KUDU-3326 disable compaction for soft-deleted table ...................................................................... Patch Set 4: (11 comments) Thanks for your reviews. http://gerrit.cloudera.org:8080/#/c/20178/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20178/3//COMMIT_MSG@9 PS3, Line 9: > nit: Remove the extra space. Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/client/client-test.cc@5396 PS3, Line 5396: } > nit: remove the blank line. Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/client/client-test.cc@5405 PS3, Line 5405: ASSERT_OK(client_->ListTables(&tables)); > Is the ASSERT_EVENTUALLY necessary? Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/client/client-test.cc@5419 PS3, Line 5419: u.table.disa > nit: non solf deleted? Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.h@1001 PS3, Line 1001: compaction. > What does the "GC algorithm" here? I meant 'compaction' and I've update the comment. http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.cc@2639 PS3, Line 2639: soft-deleted > How about unifying the name as "soft-deleted"? Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.cc@2901 PS3, Line 2901: recall delet > recalled ? Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.cc@3396 PS3, Line 3396: r > nit: remove the extra space. Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.cc@6044 PS3, Line 6044: > How about use the delete_timestamp field of "l.mutable_data()->pb" ? Done http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/catalog_manager.cc@6045 PS3, Line 6045: _delete_time > Isn't the state of table changed in L2598, why change it again? Note the comment at L2662. In the process of soft deletion, we usually do not allow modifications to the configuration of a table that is in the soft deleted state. However, if we want to disable compaction for a soft deleted table, we have to modify its configuration. Therefore, in this patch, I added a parameter to modify the configuration value of the table in the last step of the soft deletion process. However, this will result in the table's state being changed to normal, which is not what we expected. Therefore, we need to modify the table's state again here. In summary, the state modification at line 2598 is to put the table in the soft deleted state, exhibiting specific features such as disallowing modifications. The state modification here is to address the issue of the table remaining in a normal state after disabling compaction for a soft deleted table. http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/master-test.cc File src/kudu/master/master-test.cc: http://gerrit.cloudera.org:8080/#/c/20178/3/src/kudu/master/master-test.cc@a3682 PS3, Line 3682: > Why change these tests? Because the new soft deletion process has added an RPC for altering tables, we need to modify the unit tests here to address the issue of not covering the RPC process. -- To view, visit http://gerrit.cloudera.org:8080/20178 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I060810051613a19e6f5c6506effda2d698528839 Gerrit-Change-Number: 20178 Gerrit-PatchSet: 4 Gerrit-Owner: KeDeng <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 15 Aug 2023 03:54:08 +0000 Gerrit-HasComments: Yes
