Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/15145 )
Change subject: WIP KUDU-1625: background op to GC ancient, empty rowsets ...................................................................... Patch Set 1: (4 comments) Did you explore building this heuristic into the compaction perf score and letting the compaction op "delete" the rowsets? Curious what the trade-offs are between this approach and that one. http://gerrit.cloudera.org:8080/#/c/15145/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/15145/1//COMMIT_MSG@12 PS1, Line 12: - For the sake of simplicity on bootstrap, the rowset should not have a : DMS. I don't see a clear way to rebuild a DMS if the underlying DRS : has been deleted. Not quite following this: if the rowset has a DMS and we decide to delete it, it's because the deltas in the DMS are all too old, right? The rowset is then gone from the tablet, so what's the complication on bootstrap? http://gerrit.cloudera.org:8080/#/c/15145/1/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/15145/1/src/kudu/tablet/delta_tracker.cc@457 PS1, Line 457: return newest_redo->Initted() && Can newest_redo be null? If not, enforce via DCHECK? http://gerrit.cloudera.org:8080/#/c/15145/1/src/kudu/tablet/memrowset.h File src/kudu/tablet/memrowset.h: http://gerrit.cloudera.org:8080/#/c/15145/1/src/kudu/tablet/memrowset.h@400 PS1, Line 400: *empty_and_ancient = false; While we could theoretically determine whether all the mutations in the MRS are older than the AHM, there'd be no point since we won't GC it. Is this the right place to indicate that? Or should IsEmptyAndFullyAncient return NotSupported or some such and there'd be a different lever for rowset implementations to indicate that they don't participate in this GC? Same comment applies to the mock/duplicating rowsets I guess. http://gerrit.cloudera.org:8080/#/c/15145/1/src/kudu/tablet/tablet_mm_ops.h File src/kudu/tablet/tablet_mm_ops.h: http://gerrit.cloudera.org:8080/#/c/15145/1/src/kudu/tablet/tablet_mm_ops.h@162 PS1, Line 162: class EmptyRowsetGCOp : public TabletOpBase { "Empty" suggests a rowset without rows, but this is actually a rowset whose rows are all deleted. Maybe something "FullyDeleted" or "AllDeleted"? -- To view, visit http://gerrit.cloudera.org:8080/15145 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I696e2a29ea52ad4e54801b495c322bc371787124 Gerrit-Change-Number: 15145 Gerrit-PatchSet: 1 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 31 Jan 2020 20:21:27 +0000 Gerrit-HasComments: Yes
