Ensure divergent replies are ancestors when reply with needs repair #27

Merged
merged 1 commit into from Jan 6, 2012

Conversation

Projects
None yet
3 participants
Contributor

bdionne commented Dec 13, 2011

BugzID:13003

Owner

kocolosk commented Dec 30, 2011

The patch looks right to me, but I'd like to see an expanded commit message explaining what's being fixed. It's a bit subtle. Consider the set of worker responses (1-foo, 2-bar, 1-foo) where 2-bar is the direct descendant of 1-foo. Before the patch we would reply 1-foo and asynchronously execute a read repair to bring all copies up to 2-bar. We weren't checking that the reply which met quorum was the same as the lone reply left after we removed all the ancestors. That's bad -- a user who does a read-modify-write cycle is going to get an unexpected conflict on the write.

The patch causes the 1-2-1 case to fall through to the final blocking repair clause where we'll return 2-bar after the other copies have been updated. The only time we match on the fast async repair clause should be when we have something like (1-foo, 2-bar, 2-bar); i.e., we've got a read quorum, and any divergent replies are ancestors of the quorum reply.

Bob Dionne
Ensure response with needs repair contains the correct value
For example we may have responses like (1-foo, 2-bar, 1-foo) where we've
reached a quorum with 1-foo but 1-foo is an ancestor of 2-bar which is the
correct answer. In this case we don't want to respond with 1-foo. This patch
ensures that after remove_ancestors is called the remaining response is the
same as the one that made quorum. So (1-foo, 2-bar, 2-bar) would respond with
2-bar needs repair and (1-foo, 2-bar, 1-foo) would block with read repair.

BugzID:13003
Contributor

bdionne commented Dec 30, 2011

ok, I reworked this commit message. See if this is clear enough.

Owner

kocolosk commented Dec 30, 2011

That's good enough for me, but since I was pretty involved in this bug I'll wait for another +1 before merging.

Owner

kocolosk commented Jan 4, 2012

Bump. Could use a second reviewer on this one.

Owner

davisp commented Jan 6, 2012

LGTM. The commit message illuminates this enough to put it all together.

kocolosk added a commit that referenced this pull request Jan 6, 2012

@kocolosk kocolosk merged commit 910cac9 into master Jan 6, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment