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 3: (3 comments) http://gerrit.cloudera.org:8080/#/c/13821/3/src/kudu/tablet/delta_tracker.h File src/kudu/tablet/delta_tracker.h: http://gerrit.cloudera.org:8080/#/c/13821/3/src/kudu/tablet/delta_tracker.h@374 PS3, Line 374: // Number of deleted rows for a DMS that is currently being flushed. Nit: what changed here? 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_; > The "log_anchor_registry_" will be a wild pointer if we use a raw point her I don't understand your explanation. When log_anchor_registry_ was a raw pointer, it pointed to the scoped_refptr owned by Tablet. That's safe because we can't destroy Tablet without first destroying all of its DiskRowSets and DeltaTrackers. This is a fundamental tablet lifecycle property that doesn't change even when DMSes are lazily constructed. It's frustrating that LogAnchorRegistry isn't treated consistently as a shared object, but I think that's partly done for performance reasons. When Foo has a scoped_refptr<Bar> instead of a Bar*, Bar's reference count is incremented when Foo is constructed and decremented when Foo is destructed. For LogAnchorRegistry, we know that a Tablet will outlive all of its sub-objects, and because there can be many of those sub-objects (i.e. a handful per rowset), storing LogAnchorRegistry* in them means avoiding excessive increments and decrements without sacrificing safety. 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, advanced > Should I keep this gflag or just discard it? If yes, maybe some test cases I'd discard it; it doesn't seem important enough to expose. -- 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: 3 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 15:58:18 +0000 Gerrit-HasComments: Yes
