Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/15145 )
Change subject: KUDU-1625: background op to GC ancient, fully deleted rowsets ...................................................................... Patch Set 7: (7 comments) http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/integration-tests/tablet_history_gc-itest.cc File src/kudu/integration-tests/tablet_history_gc-itest.cc: http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/integration-tests/tablet_history_gc-itest.cc@121 PS7, Line 121: client:: > Could 'using' this. Done http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/integration-tests/tablet_history_gc-itest.cc@308 PS7, Line 308: const int32_t kNumRows = AllowSlowTests() ? 100 : 10; > Is this really significant enough that it warrants the slow test conditioni Done http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/integration-tests/tablet_history_gc-itest.cc@323 PS7, Line 323: uint64_t measured_size_before_gc = 0; > Nit: don't need to initialize this or after_gc. Done http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/integration-tests/tablet_history_gc-itest.cc@335 PS7, Line 335: ASSERT_LT(measured_size_after_gc, measured_size_before_gc); > Hole punching is asynchronous, so you might want to ASSERT_EVENTUALLY here. Went with ASSERT_EVENTUALLY. I presume compaction-test was written before that was a thing. http://gerrit.cloudera.org:8080/#/c/15145/4/src/kudu/tablet/tablet-test-base.h File src/kudu/tablet/tablet-test-base.h: http://gerrit.cloudera.org:8080/#/c/15145/4/src/kudu/tablet/tablet-test-base.h@342 PS4, Line 342: // Deletes 'count' rows, starting with 'first_row'. > Did you miss the other comment? Or disagree? Ah, I missed it. http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/tablet/tablet.cc@2391 PS7, Line 2391: LOG(INFO) << LogPrefix() << Substitute("no rowsets deleted: skipped $0 rowsets " : "that are being compacted", num_unavailable_for_delete); > Do you think this is valuable to print? What other op-related stuff gets pr Just the "scheduled" and "completed" messages are logged. This was helpful to debug some concurrency problems that are fixed by checking IsAvailableForCompaction() when estimating. Not sure there's much value now though, so I'll remove this. http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/tablet/tablet_mm_ops.cc File src/kudu/tablet/tablet_mm_ops.cc: http://gerrit.cloudera.org:8080/#/c/15145/7/src/kudu/tablet/tablet_mm_ops.cc@84 PS7, Line 84: TAG_FLAG(enable_deleted_rowset_gc, experimental); > BTW, sort of odd that it defaults to true but you need --unlock-experimenta Done -- 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: 7 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Volodymyr Verovkin <[email protected]> Gerrit-Comment-Date: Fri, 20 Mar 2020 19:34:48 +0000 Gerrit-HasComments: Yes
