David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1601. Delete ancient UNDO delta blocks in the background ......................................................................
Patch Set 12: (14 comments) http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tablet/tablet_history_gc-test.cc File src/kudu/tablet/tablet_history_gc-test.cc: PS12, Line 550: // No deletes. why is this relevant? PS12, Line 559: MonoTime deadline = MonoTime::Now() + MonoDelta::FromSeconds(60); use MonoTime::Max() here PS12, Line 560: bytes_in_ancient_undos assert that this is GT 0 http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tablet/tablet_metrics.cc File src/kudu/tablet/tablet_metrics.cc: PS12, Line 119: Does not include bytes garbage collected during compactions. why? think it would make sense to have both, no? PS12, Line 126: Does not include blocks garbage collected during compactions. not sure it's worth it to have both metrics. what does the number of blocks tell you that number of bytes doesn't? http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tserver/tablet_service.cc File src/kudu/tserver/tablet_service.cc: PS12, Line 1131: WARN_NOT_OK(server_->tablet_manager()->GetTabletPeers(&peers), "Cannot list tablets" return on not OK? http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tserver/ts_tablet_manager.cc File src/kudu/tserver/ts_tablet_manager.cc: PS12, Line 842: WARN_NOT_OK(GetTabletPeers(&peers_to_shutdown), "Cannot list tablets"); should this be a warn? what happens if we don't get all of them? from what I get of the impl it always returns Status::OK http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tserver/ts_tablet_manager.h File src/kudu/tserver/ts_tablet_manager.h: PS12, Line 190: fs_manager docs http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tserver/tserver-path-handlers.cc File src/kudu/tserver/tserver-path-handlers.cc: PS12, Line 114: WARN_NOT_OK(tserver_->tablet_manager()->GetTabletPeers(&peers), "Cannot list tablets"); what happens below on not-OK? PS12, Line 182: WARN_NOT_OK(tserver_->tablet_manager()->GetTabletPeers(&peers), "Cannot list tablets"); same http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tserver/tserver_mm_ops.cc File src/kudu/tserver/tserver_mm_ops.cc: PS12, Line 59: Total Undo Delta Block GC Init Duration think the explanation text below is better than the metric name and not much longer, same for some of the other metrics PS12, Line 108: enable_undo_delta_block_gc same comment as before regarding merging the enable_ flag with the undo max age flag PS12, Line 176: wih typo http://gerrit.cloudera.org:8080/#/c/4363/12/src/kudu/tserver/tserver_mm_ops.h File src/kudu/tserver/tserver_mm_ops.h: PS12, Line 50: class UndoDeltaBlockGCOp : public MaintenanceOp likely can make this a regular tablet op with the changes we discussed. -- To view, visit http://gerrit.cloudera.org:8080/4363 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0309bf7acfb6d018860c80f354012c3500da5c68 Gerrit-PatchSet: 12 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: David Ribeiro Alves <dral...@apache.org> Gerrit-Reviewer: Jean-Daniel Cryans <jdcry...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
