Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8859 )
Change subject: KUDU-2115: remove unnecessary compaction selection check ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/8859/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/8859/3//COMMIT_MSG@19 PS3, Line 19: rowsets and the verification, the component_lock_ is dropped. As such, : the verification hits false negatives if other threads are able to take : the component_lock_ and compact the available rowsets. The race wasn't immediately clear to me from the commit message but I looked at the code and think I get it. To verify that I'm understanding correctly, is the race as follows? T1: runs PickRowSetsToCompact, picks {A, B}, locks them T1: begins compaction T2: starts running PickRowSetsToCompact T2: - snapshots the tree (which still contains {A, B}) T2: - drops the component_lock after snapshot T1: finishes the compaction and removes {A, B} from the RowSetTree and unlocks their compact_flush_lock T2: - iterates over the rowsets and sees {A, B} as "IsAvailableForCompaction" (because they've become unlocked) T2: - selects them for compaction, but they are no longer in the tree (FATALs) if so, can you add this to the commit message? http://gerrit.cloudera.org:8080/#/c/8859/3//COMMIT_MSG@24 PS3, Line 24: not returned by PickRowSetsToCompact() anyway. I've removed this check. Ignoring and proceeding could have the side effect of running a fairly useless compaction. Instead, could we either: (a) move the filtering by IsAvailableForCompaction from RowSetInfo::CollectOrdered() into Tablet::PickRowSetsToCompact() while holding the component lock? That way we wouldn't possibly select a RowSet which has been later removed from the tree. (this would require some signature changes on CompactionPolicy to just take a vector of RowSets instead of a RowSetTree but that's probably fine) or (b) have some flag in the RowSet like 'removed' and make IsAvailableForCompaction check both the compact_flush_lock_ and the 'removed_' flag? That way in the case that something is removed from the tree, the flag would get set and prevent it from getting selected in a later compaction policy invocation. -- To view, visit http://gerrit.cloudera.org:8080/8859 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4dab330d61facb18717f6faf179f9b94a9e55236 Gerrit-Change-Number: 8859 Gerrit-PatchSet: 3 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 21 Dec 2017 17:32:48 +0000 Gerrit-HasComments: Yes
