Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/18503 )
Change subject: [KUDU-3367][compaction] Fix delta file with full of delete op can not be schedule to compact ...................................................................... Patch Set 27: (5 comments) 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] Fix delta file with full of delete op can not be schedule to compact : : 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. : : See https://issues.apache.org/jira/projects/KUDU/issues/KUDU-3367 for details. Please follow the guidelines outlined at https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project#_commit_guidelines The link to the guide could be found at https://kudu.apache.org/docs/contributing.html#_submitting_patches 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: // Lock protecting the last_update_time_ below. : mutable rw_spinlock update_time_lock_; Could we use std::atomic<MonoTime> for last_update_time_ and avoid introducing locks? 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 name of variable/parameter 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; : CHECK_OK(MutateRow(rs, : i, : RowChangeList(delete_buf), : &result)); : CHECK_EQ(1, result.mutated_stores_size()); : CHECK_EQ(rs->metadata()->id(), result.mutated_stores(0).rs_id()); ASSERT_xx is used above, maybe switch to using ASSERT instead of CHECK here as well? 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: DeleteExistingRows Should this be wrapped into NO_FATALS()? -- 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: 27 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: Tue, 06 Dec 2022 20:45:19 +0000 Gerrit-HasComments: Yes
