KeDeng has posted comments on this change. ( http://gerrit.cloudera.org:8080/18503 )
Change subject: KUDU-3367 [compaction] supplement to gc algorithm ...................................................................... Patch Set 28: (5 comments) Thanks for your reviews. http://gerrit.cloudera.org:8080/#/c/18503/27//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/18503/27//COMMIT_MSG@7 PS27, Line 7: KUDU-3367 [compaction] supplement to gc algorithm : : If we get a REDO delta with full of delete op, which means there is : no update op else in the file. The current compact algorithm will : not schedule the file do compact. If such files exist, after : accumulating for a period of time, it will greatly affect our scan : speed. > Please follow the guidelines outlined at https://git-scm.com/book/en/v2/Dis Done http://gerrit.cloudera.org:8080/#/c/18503/27/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/18503/27/src/kudu/tablet/delta_tracker.h@399 PS27, Line 399: // As a supplement to KUDU-1625, we need release storage space for old : // tablet metadata that does not suppo > Could we use std::atomic<MonoTime> for last_update_time_ and avoid introduc Done http://gerrit.cloudera.org:8080/#/c/18503/27/src/kudu/tablet/diskrowset-test-base.h File src/kudu/tablet/diskrowset-test-base.h: http://gerrit.cloudera.org:8080/#/c/18503/27/src/kudu/tablet/diskrowset-test-base.h@156 PS27, Line 156: DiskRowSet* > style nit here and elsewhere: stick the asterisk to the type, not to the na Done http://gerrit.cloudera.org:8080/#/c/18503/27/src/kudu/tablet/diskrowset-test-base.h@163 PS27, Line 163: for (int i = start_idx; i < end_idx; i++) { : update.Reset(); : update.SetToDelete(); : OperationResultPB result; : ASSERT_OK(MutateRow(rs, : i, : RowChangeList(delete_buf), : &result)); : ASSERT_EQ(1, result.mutated_stores_size()); : ASSERT_EQ(rs->metadata()->id(), result.mutated_stores(0).rs_id()) > ASSERT_xx is used above, maybe switch to using ASSERT instead of CHECK here Done http://gerrit.cloudera.org:8080/#/c/18503/27/src/kudu/tablet/diskrowset-test.cc File src/kudu/tablet/diskrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/18503/27/src/kudu/tablet/diskrowset-test.cc@673 PS27, Line 673: NO_FATALS(DeleteEx > Should this be wrapped into NO_FATALS()? Done -- To view, visit http://gerrit.cloudera.org:8080/18503 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8b26737dffecc17688b42188da959b2ba16351ed Gerrit-Change-Number: 18503 Gerrit-PatchSet: 28 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: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Wed, 07 Dec 2022 02:53:10 +0000 Gerrit-HasComments: Yes
