Yingchun Lai has posted comments on this change. ( http://gerrit.cloudera.org:8080/19670 )
Change subject: [GC] gc ancient, fully deleted rowsets without live row count ...................................................................... Patch Set 5: (6 comments) http://gerrit.cloudera.org:8080/#/c/19670/5/src/kudu/tablet/diskrowset-test.cc File src/kudu/tablet/diskrowset-test.cc: http://gerrit.cloudera.org:8080/#/c/19670/5/src/kudu/tablet/diskrowset-test.cc@771 PS5, Line 771: TestCountLiveRowsWithoutLiveRowCount I think it's necessary to test that when the rows of this DRS have been fully deleted, CountLiveRows is able to get 0, and CountLiveRowsWithoutLiveRowCount is able to get a value in [0, inserted_row_count], and it would be nice to get a value less than inserted_row_count. http://gerrit.cloudera.org:8080/#/c/19670/5/src/kudu/tablet/diskrowset-test.cc@773 PS5, Line 773: WriteTestRowSet(); The function declaration is void WriteTestRowSet(int n_rows = 0, bool zero_vals = false) Is it what you want to write 0 rows to the rowset? http://gerrit.cloudera.org:8080/#/c/19670/5/src/kudu/tablet/diskrowset-test.cc@780 PS5, Line 780: CHECK_OK(rs->CountLiveRows(&rs_live_rows)); nit: check the accurate value of 'rs_live_rows' http://gerrit.cloudera.org:8080/#/c/19670/5/src/kudu/tablet/diskrowset-test.cc@790 PS5, Line 790: CHECK_OK(rs->CountLiveRows(&rs_live_rows)); nit: Check 'rs_live_rows' is not change after some update operations. http://gerrit.cloudera.org:8080/#/c/19670/5/src/kudu/tablet/diskrowset-test.cc@801 PS5, Line 801: ASSERT_LE(rs_live_rows_without_lrc, rs_live_rows); Also to check that 'rs_live_rows_without_lrc' is larger than some value? http://gerrit.cloudera.org:8080/#/c/19670/5/src/kudu/tablet/diskrowset-test.cc@802 PS5, Line 802: MajorCompactDeltaStores I'm curious what's the purpose of this compact in this test? -- To view, visit http://gerrit.cloudera.org:8080/19670 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iacdff107b8b07cbd56f47f296a93f4bcfbf56b41 Gerrit-Change-Number: 19670 Gerrit-PatchSet: 5 Gerrit-Owner: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: KeDeng <kdeng...@gmail.com> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <laiyingc...@apache.org> Gerrit-Comment-Date: Mon, 03 Apr 2023 16:11:27 +0000 Gerrit-HasComments: Yes