helifu 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: (8 comments) 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? The "log_anchor_registry_" will be a wild pointer if we use a raw point here because it will be release by "registry_" from class MinLogIndexAnchorer. For example, TestRowSet.TestCompactStores from diskrowset-test.cc: 1) the object "log::LogAnchorRegistry" is a raw pointer: https://github.com/apache/kudu/blob/3cbc0d4fbe295748d6ffdf1e5e7edeaf94ef0911/src/kudu/tablet/diskrowset-test-base.h#L330 2) the raw pointer will be released by "registry_" from class MinLogIndexAnchorer 3) then the next creation for DMS is dangerous. ================== On the other side, yes, we can use a raw pointer indeed. But I think it's better to use a ref_ptr because we can't ensure that every caller retains its lifecycle. http://gerrit.cloudera.org:8080/#/c/13821/2/src/kudu/tablet/delta_tracker.h@352 PS2, Line 352: dms_exist_ > Nit: dms_exists_ Done 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 Done 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? Should I keep this gflag or just discard it? If yes, maybe some test cases to cover true or false are necessary. 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 Done 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 Emmm, the DMS will be not returned when dms_exists_ is false either. And "!dms_->Empty" can help to avoid returning a empty DMS, though that is unlikely to happen. 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. Done 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_" Done -- 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-Reviewer: helifu <[email protected]> Gerrit-Comment-Date: Wed, 10 Jul 2019 11:40:33 +0000 Gerrit-HasComments: Yes
