Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/13821 )
Change subject: KUDU-2855 Lazy-create DeltaMemStore on first update ...................................................................... Patch Set 2: (9 comments) http://gerrit.cloudera.org:8080/#/c/13821/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/13821/2//COMMIT_MSG@9 PS2, Line 9: DeltaMemStrore DeltaMemStore http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.h@336 PS2, Line 336: const scoped_refptr<log::LogAnchorRegistry> log_anchor_registry_; Why can't this remain a raw pointer? http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.h@352 PS2, Line 352: dms_exist_ Nit: dms_exists_ http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.h@356 PS2, Line 356: // - Mutators take this lock in exclusive mode while dms_ is not initialized. "Mutators take this lock in exclusive mode if they need to create a new DMS, and shared mode otherwise." http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.cc File src/kudu/tablet/delta_tracker.cc: http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.cc@78 PS2, Line 78: DEFINE_bool(dms_lazy_create, true, : "Allow lazily creating of DeltaMemStore"); : TAG_FLAG(dms_lazy_create, hidden); What's the purpose of this if it's hidden? http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.cc@169 PS2, Line 169: Status DeltaTracker::CreateAndInitDMSUnlocked(const fs::IOContext* io_context) { This should only be done with component_lock_ held in exclusive mode, right? could you DCHECK on that? And take the lock before L162? http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.cc@601 PS2, Line 601: !dms_->Empty() This seems like a semantic change in that previously CollectStores returned the DMS even if it was empty. Is it safe? http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.cc@659 PS2, Line 659: Status s = Status::OK(); This is the default value of a Status. http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.cc@673 PS2, Line 673: operations of the above component_lock_ "critical sections defined by component_lock_" -- To view, visit http://gerrit.cloudera.org:8080/13821 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie0c565d86647d5144266b30aa6e8572d42db48c6 Gerrit-Change-Number: 13821 Gerrit-PatchSet: 2 Gerrit-Owner: helifu <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Wed, 10 Jul 2019 03:58:13 +0000 Gerrit-HasComments: Yes
