Todd Lipcon has posted comments on this change. Change subject: KUDU-1807: improve IsCreateInProgress performance ......................................................................
Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/7957/3/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 554 hrm, why'd you remove the anonymous namespace here? actually it appears it should probably be higher up to encompass the visitors as well? PS3, Line 634: the same order as tablet : // lock acquisition I thought we usually acquire the table lock first, but here we are acquiring the tablet lock in commit mode before we are trying to acquire some table-level lock in ApplyTabletStateChange, no? PS3, Line 639: const auto* change = FindOrNull(state_changes_, t); : if (change) { : t->mutable_metadata()->CommitMutation([&]() { : t->table()->ApplyTabletStateChange(change->first, change->second); : }); : } else { : t->mutable_metadata()->CommitMutation(); : } hrm, I think this might be easier to read as: t->mutable_metadata()->CommitMutation([&]() { if (const auto* change = FindOrNull(state_changes_, t)) { t->table()->ApplyTabletStateChange(change->first, change->second); } }); i.e fold the branch inside the lambda rather than outside, so you get rid of the multiple call sites to CommitMutation PS3, Line 3747: unlocker_out update this name Line 4573: VLOG(3) << Substitute("$0: incrementing tablet state $1", this log might be more useful if it included either the old or new value of the count Line 4582: VLOG(3) << Substitute("$0: decrementing tablet state $1", same PS3, Line 4589: DCHECK I think this is worth a CHECK because otherwise we will be decrementing some random memory Line 4592: if (it->second == 0) { maybe DCHECK_GE(it->second, 0)? PS3, Line 4597: VerifyTabletStateCountsUnlocked maybe expand this to also verify teh schema version counts since it's a similar use case? http://gerrit.cloudera.org:8080/#/c/7957/3/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: PS3, Line 298: ApplyTabletStateChange how about AccountTabletStateChange since it's only accounting and not actually changing any states? PS3, Line 739: tablet_lock change param name -- To view, visit http://gerrit.cloudera.org:8080/7957 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idca379bb1249e54c0b7fbb0df408ed829ab9fbd2 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
