Mike Percy has posted comments on this change. Change subject: KUDU-1601. Delete ancient UNDO delta blocks in the background ......................................................................
Patch Set 9: (32 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? Done 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 I'll just remove the comment. PS9, Line 116: is fine > s/is fine/cluster Done 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 Done PS9, Line 148: j > s/j/row_value Done 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 value Done 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:") Done Line 730: // CatalogManager-wide maintenance operations. > nit: prepend "Start/Stop " Done Line 771: std::vector<std::unique_ptr<MaintenanceOp>> maintenance_ops_; > docs Done 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 Done 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 dele The blocks don't actually get deleted until we flush the metadata, since that is when the orphans list gets traversed. 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 refe I was trying to say that we shouldn't use it for mutations because the flush happens last. If we aren't adding mutations then the ordering doesn't matter much... we could never explicitly flush the metadata and still maintain correctness. PS9, Line 165: max_timestamp > surround with ' for consistency (or remove the underscore) Removed the underscore PS9, Line 173: Timestamp::kInvalidTimestamp > maybe we should use kInitialTimestamp instead (kmin +1 instead of kMax -1), Why do you think that helps with comparison mistakes? If we say invalid == super old then if there was a bug we might accidentally to Init (or GC) newer deltas. PS9, Line 176: If no return value is desired > this reads weird Any suggestions? I tried to improve this. PS9, Line 178: InitAncientUndoDeltas > is this always called independently of whether we are GC'ing undos? if so w Not really, but OK that seems fine. 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 sta Done PS9, Line 228: not including the DeltaMemStore. > the deltamemstore is alway REDO, this doesn't make much sense Done PS9, Line 244: WhichStores > this "WhichStores" seems to imply it's not "all" Updated the comment. If you are referring to the name of the type, it's an already-existing type and I don't think it's strictly wrong to say something like: "Which of these men robbed the bank?" "All of them." PS9, Line 298: LogPrefix > docs I don't think it's necessary or desirable to doc this because it's for LOG_WITH_PREFIX() and it's everywhere throughout the code base these days. 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 Done 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? Yep! 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 Hmm. You're right, this wasn't consistent. I'm making it consistent with the other one, with an explanation. http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/tserver/tserver_mm_ops.cc File src/kudu/tserver/tserver_mm_ops.cc: PS9, Line 34: . > maybe add "(doesn't prevent compactions from deleting ancient data)" Done, with a minor tweak to that wording PS9, Line 37: DEFINE_int32(undo_delta_block_gc_init_budget_millis, 100, > this should likely be tagged as advanced, maybe even hidden. How about evolving + advanced? PS9, Line 43: DEFINE_double(undo_delta_block_gc_delete_max_selection_ratio, 0.5, : "The maximum fraction of the time that undo delta block gc may be selected " : "to run by the maintenance manager. This is measured by the ratio of number " : "of times UpdateStats() is called over the number of times Perform() is " : "called on the UndoDeltaBlockGCOp. This is intended to avoid that task " : "starving other maintenance tasks."); > tags (hidden, likely). this is hard to understand as a prioritization cap. Done http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/tserver/tserver_mm_ops.h File src/kudu/tserver/tserver_mm_ops.h: PS9, Line 1: // Licensed to the Apache Software Foundation (ASF) under one > Not sure I got the complete picture of all the discussions and the controve I don't really understand what you mean by this. Can you clarify? PS9, Line 55: OVERRIDE > use c++11 overrides Done PS9, Line 65: LogPrefix > docs See my other comment on this; It's so prevalent I don't think docs provide any value here. http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/util/maintenance_manager.cc File src/kudu/util/maintenance_manager.cc: PS9, Line 69: The target minimum size (in megabytes), per tablet, of ancient " : "data on disk needed to prioritize deletion of that ancient data. > this sentence is very hard to parse. also why do we need a min? if we have The continuous spectrum thing makes sense in theory but isn't straightforward in practice. What maps to 0.5? The simple heuristic I am using right now, by default, is that if the amount of data to GC is > 0, we attempt to run GC (if it's runnable). We prevent GC from being runnable using the max_selection_ratio limit in tserver_mm_ops.cc We could potentially get rid of this but I thought it might come in handy some day in the field. I marked it experimental since we also might want to just remove it some day. http://gerrit.cloudera.org:8080/#/c/4363/9/src/kudu/util/maintenance_manager.h File src/kudu/util/maintenance_manager.h: PS9, Line 118: Important: Update Clear() when adding fields to this class. > nice add, but could you move it to the class header? Wouldn't developers adding fields to this class be more likely to see it here? PS9, Line 135: The approximate amount of disk space that not doing this operation keeps us from GCing from : // the data blocks. > I couldn't parse this, I know its a parallel comment to the fields above, b Changed it to something less strange, as well as the one above it. -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Jean-Daniel Cryans <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-HasComments: Yes
