Adar Dembo has posted comments on this change. Change subject: KUDU-1807: improve IsCreateInProgress performance ......................................................................
Patch Set 2: (11 comments) > 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) I added a function to do this verification, and added a call to it in IsCreateInProgress. Let me know if you think I should call it elsewhere, or add something else. 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 GetTableLo You're right that there's a race window here, and that it can cause GetTableLocations to think that a tablet isn't yet running. However, all of our client implementations will treat this as a transient state and retry with some backoff. Given the above, I wasn't sure whether this is worth fixing. What ultimately convinced me was that it's needed for the correctness of a "compare the tablet state count map contents against the actual state counts" function. PS2, Line 663: set_state > perhaps this should be named swap_state to indicate its return value better Done Line 665: state_changes_[tablet->table()][old_state]--; > can we DCHECK here that this never goes negative? On the contrary: it probably will be negative, because state_changes_ represents the deltas that will be applied rather than the absolute counts. In any case, this is moot now because we're no longer aggregating state changes for tables. Line 672: const PersistentTabletInfo& data( > nit: is this wrapping necessary? I like to wrap at 80, and these two lines were 84 and 86 respectively. But I'll unwrap since you found it distracting. Line 681: PersistentTabletInfo* mutable_data( > nit: same (wrapping)? Done PS2, Line 3697: unlocker_in > maybe 'committer' is a better name Done, though I preserved the 'in' and 'out' suffixes to help navigate between these functions and ProcessPendingAssignments. PS2, Line 4468: SysTabletsEntryPB::RUNNING > don't we have to worry about REPLACED and DELETED? or do those actually get I tried to maintain the same semantics as the old implementation, which returned true when it found any non-RUNNING tablet. A tablet switches to REPLACED in ProcessPendingAssignments but is implicitly removed from the map when AddRemoveTablets is used to add the actual replacement tablet. It's interesting that here the mutation is committed before AddRemoveTablets is called, so there's a very brief window where a REPLACED tablet could be visible. I'm guessing this is harmless because it also implies a CREATING tablet, which means the table is definitely still being created. A tablet switches to DELETED either in DeleteTable or in AlterTable (when a range partition is dropped). In the former, the entire table switches to DELETED, which causes IsCreateInProgress callers to short-circuit. In the latter, a DELETED tablet is explicitly dropped via AddRemoveTablets. And unlike ProcessPendingAssignments, the mutation is committed after the tablet is dropped, so the DELETED tablet is never visible. PS2, Line 4591: DCHECK_GT(it->second, 0); > can you DCHECK_GE() vs 'delta'? Moot, Increment/Decrement now always add/remove just one. 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 It's because the three callers are all tablet->metadata().state().get_state(). So if this were just state(), the calls would be tablet->metadata().state().state(), which I found confusing. I can still change this if you prefer. PS2, Line 244: TabletStateMap > how about TabletStateCountMap? otherwise I would expect this type to be key Done 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 separa My original implementation used a single method which allowed negative deltas, but then I saw that the implementation was pretty different for negative deltas (due to additional invariant enforcement), and the symmetry with the SchemaVersionCount methods looked nice. Anyway, this is moot now because per-table state change batching is no longer a thing, so we always increment/decrement one at a time. -- 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: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
