Todd Lipcon has posted comments on this change. Change subject: KUDU-1807: improve IsCreateInProgress performance ......................................................................
Patch Set 2: (11 comments) I only partially followed this patch... it's one of those ones that's hard to verify by looking at it visually. Is there any way we can add a debug-mode consistency check of some sort that verifies that the map of counts maps the calculated iteration? even if it's correct right now it seems like it may regress and be really hard to debug at some point later. (iirc the Java client just loops forever trying to fetch locations if it thinks the table is in-progress) http://gerrit.cloudera.org:8080/#/c/7957/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 637: // Then, commit the mutations. is there some race window here where a caller ends up asking for GetTableLocations and it says that create is not in-progress, but in fact some of the tablets still haven't gotten to the Commit() step below, and hence are not returned? PS2, Line 663: set_state perhaps this should be named swap_state to indicate its return value better Line 665: state_changes_[tablet->table()][old_state]--; can we DCHECK here that this never goes negative? Line 672: const PersistentTabletInfo& data( nit: is this wrapping necessary? Line 681: PersistentTabletInfo* mutable_data( nit: same (wrapping)? PS2, Line 3697: unlocker_in maybe 'committer' is a better name (same below) PS2, Line 4468: SysTabletsEntryPB::RUNNING don't we have to worry about REPLACED and DELETED? or do those actually get entirely removed? PS2, Line 4591: DCHECK_GT(it->second, 0); can you DCHECK_GE() vs 'delta'? http://gerrit.cloudera.org:8080/#/c/7957/2/src/kudu/master/catalog_manager.h File src/kudu/master/catalog_manager.h: PS2, Line 123: get_state unusual naming for a getter PS2, Line 244: TabletStateMap how about TabletStateCountMap? otherwise I would expect this type to be keyed by tablet id like the TabletInfo one. PS2, Line 318: void IncrementTabletStateCountUnlocked(SysTabletsEntryPB::State state, : int64_t delta = 1); : void DecrementTabletStateCountUnlocked(SysTabletsEntryPB::State state, : int64_t delta = 1); if you are taking counts here, it seems a little odd to have the two separate methods instead of just having one, and allowing a negative 'delta' -- 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: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
