You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Mike Percy (Code Review)" <ge...@cloudera.org> on 2017/08/17 23:11:30 UTC
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Hello David Ribeiro Alves, Todd Lipcon, Alexey Serbin,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7718
to review the following change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
consensus: Tablet copy should clear last-logged opid from superblock
Keeping around irrelevant information is bad hygiene. Plus, we don't
want to disk tombstoned voting using the wrong last-logged opid from the
superblock while there is a last-logged opid stored in the WAL.
Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
3 files changed, 66 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/7718/1
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 5: Verified+1
The failure in this build was caused by a host error, as seen in the stderr log from dist-test. I looped maintenance_manager-test to be sure and it's not flaky at all, even under TSAN with stress.
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 4: Code-Review+2
Got it, sorry for the confusion.
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7718/2//COMMIT_MSG
Commit Message:
PS2, Line 10: want to disk tombstoned voting
> ?
s/disk/risk/ but I'm not sure it adds much. I'll just remove the last sentence.
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Todd Lipcon, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/7718
to look at the new patch set (#3).
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
consensus: Tablet copy should clear last-logged opid from superblock
Keeping around irrelevant information is bad hygiene.
Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
3 files changed, 66 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/18/7718/3
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 2: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7718/2//COMMIT_MSG
Commit Message:
PS2, Line 10: want to disk tombstoned voting
?
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7718/3//COMMIT_MSG
Commit Message:
Line 9: Keeping around irrelevant information is bad hygiene.
Mike and I discussed offline... it seems like this might not be a great idea after all as implemented here. We want to preserve the ability to vote in a case like:
- a new replica is being added
- tablet copy starts
- either the tablet copy source or destination crashes
- cluster restarts, and one of the other two replicas doesn't come back up
Now we have the following replicas:
1) good state
2) crashed / not coming back up
3) failed tablet copy
We want to make sure that replica #3 retains the ability to vote so it can elect #1 and repair itself.
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
consensus: Tablet copy should clear last-logged opid from superblock
Keeping around irrelevant information is bad hygiene.
Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Reviewed-on: http://gerrit.cloudera.org:8080/7718
Reviewed-by: Todd Lipcon <to...@apache.org>
Tested-by: Mike Percy <mp...@apache.org>
---
M src/kudu/integration-tests/tablet_copy-itest.cc
M src/kudu/tablet/tablet_metadata.cc
M src/kudu/tablet/tablet_metadata.h
3 files changed, 66 insertions(+), 0 deletions(-)
Approvals:
Mike Percy: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 4: Verified+1
mt-tablet-test is very slightly flaky due to KUDU-1736; overriding
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] consensus: Tablet copy should clear last-logged opid from superblock
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: consensus: Tablet copy should clear last-logged opid from superblock
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/7718/3//COMMIT_MSG
Commit Message:
Line 9: Keeping around irrelevant information is bad hygiene.
> Mike and I discussed offline... it seems like this might not be a great ide
The patch as-written only clears the last-logged opid when we transition to TABLET_DATA_READY, which is only synced after tablet copy is successfully completed.
So the scenario summarized above wouldn't be affected negatively by this (correctness related) patch. I definitely agree we want to preserve the last-logged opid as much as possible (for availability reasons) and I am planning on ensuring that we do that in a follow-up patch.
--
To view, visit http://gerrit.cloudera.org:8080/7718
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaa84d59c63222e9ddb05dca492f9ecd47b5c63ea
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <da...@gmail.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes