David Ribeiro Alves has posted comments on this change. Change subject: KUDU-1601. Delete ancient UNDO delta blocks in the background ......................................................................
Patch Set 9: (23 comments) http://gerrit.cloudera.org:8080/#/c/4363/9//COMMIT_MSG Commit Message: PS9, Line 15: The task was written to operate at the server level in contrast to? http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/integration-tests/tablet_history_gc-itest.cc File src/kudu/integration-tests/tablet_history_gc-itest.cc: PS9, Line 112: // Don't GC until we move the clock. nit: weird comment. isn't this more related to the increments to the clock that you make? i.e. if you didn't set this we still wouldn't GC until we moved the clock. PS9, Line 116: is fine s/is fine/cluster PS9, Line 118: // Create a single-replica tablet so we can write to it and test undo GC on it. don't think we need this comment PS9, Line 148: j s/j/row_value PS9, Line 774: ASSERT_OK(tablet->DeleteAncientUndoDeltas(&blocks_deleted, &bytes_deleted)); add some assertion that makes sure than when a value is non-zero both values are non-zero (this is correct, right?) http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: PS9, Line 501: GetTabletPeers docs (at least a "see:") Line 730: // CatalogManager-wide maintenance operations. nit: prepend "Start/Stop " Line 771: std::vector<std::unique_ptr<MaintenanceOp>> maintenance_ops_; docs http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/server/server_base.h File src/kudu/server/server_base.h: PS9, Line 101: virtual MaintenanceManager* maintenance_manager() = 0; docs http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: PS9, Line 434: RETURN_NOT_OK(CommitDeltaUpdate(update, undos_to_remove, {}, UNDO, NO_FLUSH_METADATA)); so the caller is supposed to flush the metadata _after_ the blocks are deleted? is there no problem if the caller fails to flush the metadata and the blocks are deleted? do we have coverage for that? aside: having a nice comment on the order of operations here. http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: PS9, Line 149: not : // modifications to data values. not sure what this means... which modifications to data values are you referring to? I think the ambiguity you're trying to address stems from the method name. maybe rename this to: CommitDeltaStoreMetadataUpdate or some such PS9, Line 165: max_timestamp surround with ' for consistency (or remove the underscore) PS9, Line 173: Timestamp::kInvalidTimestamp maybe we should use kInitialTimestamp instead (kmin +1 instead of kMax -1), to perclude any possible comparison mistakes also not sure I understand what you mean by "max_timestamp of each delta is not considered" PS9, Line 176: If no return value is desired this reads weird PS9, Line 178: InitAncientUndoDeltas is this always called independently of whether we are GC'ing undos? if so would likely make sense to remove "ancient" from the method name PS9, Line 189: / Does not flush updates to the rowset metadata. The caller must flush the : // metadata explicitly. should likely append "after calling this method and if it returns an OK status" PS9, Line 228: not including the DeltaMemStore. the deltamemstore is alway REDO, this doesn't make much sense PS9, Line 244: WhichStores this "WhichStores" seems to imply it's not "all" PS9, Line 298: LogPrefix docs http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/tablet/diskrowset-test.cc File src/kudu/tablet/diskrowset-test.cc: PS9, Line 568: -1 magic number, use const or add /* */ style coment, same for the nulls http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/tablet/diskrowset.cc File src/kudu/tablet/diskrowset.cc: PS9, Line 20: stl_logging.h interesting, is this to print the vector? PS9, Line 543: CHECK_OK(rowset_metadata_->Flush()); fuzzy a to why its not ok to flush here and its ok to flush in other places -- 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: 9 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
