Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/10926 )
Change subject: memrowset: support iteration with snap_to_exclude ...................................................................... Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc File src/kudu/tablet/memrowset.cc: http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@514 PS7, Line 514: bool unset_in_sel_vector this name is making me a little confused, because it can change again after it's set here. For the purposes of code clarity, do you think this could be something like: // Assume the row should be included unless we determine otherwise below. bool unset_in_sel_vector = false; bool insertion_excluded_by_snaps = ...; if (insertion_excluded_by_snaps || opts_.snap_to_include.IsCommitted(...)) { // maybe use an enum to take the out-param of ApplyMutations, like NONE_APPLIED, APPLIED_AND_DELETED, APPLIED_AND_PRESENT ApplyMutations(...); unset_in_sel_vector = mut_status == APPLIED_AND_DELETED || (mut_status == NONE_APPLIED && insertion_excluded_by_snap); } else { unset_in_sel_vector = true; } I think the code is equivalent, just trying to find something that's a little easier to follow here without trying to track this tri-state of unset_in_sel_vector through multiple functions where it might get set or left alone in various places http://gerrit.cloudera.org:8080/#/c/10926/7/src/kudu/tablet/memrowset.cc@517 PS7, Line 517: if (has_upper_bound() && out_of_bounds(k)) { : state_ = kFinished; : break; I wonder if this actually belongs outside the if condition? Even if we exclude some row due to MVCC, if we see that we've reached a key above our scan bounds, we can stop at the point instead of continuing until we find a non-excluded row (which would necesarily be out of bounds as well) (it seems like it should have been like this before as well, not new in your change, but maybe would make this look a little less complex) -- To view, visit http://gerrit.cloudera.org:8080/10926 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9be2ab5af4d5223889e4545ae1db7cc0275480ce Gerrit-Change-Number: 10926 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 24 Jul 2018 22:36:09 +0000 Gerrit-HasComments: Yes
