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 2016/08/19 08:34:47 UTC
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Mike Percy has uploaded a new change for review.
http://gerrit.cloudera.org:8080/4059
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Actually support downgrade to version that has LocalConsensus
Commit 74210b2546df9fd5dec7bb926eeb524362d2da90 attempted to support
downgrade from 0.10.x to 0.9.x however there is a CHECK that was missed
in the initial patch, and the local field must always be set.
This patch was manually tested as follows:
A 3-way replicated table was created in 0.10.0-RC1 plus this patch,
using a single master and 3 tablet servers. The services were killed,
then restarted using 0.9.1 binaries.
However, there is another downgrade problem with
1b7abe081817b5f9af979407df105a08dd226521 that is not resolved by this
patch.
Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 4 insertions(+), 1 deletion(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/4059/1
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 5:
Punted because the code was equivalent and was already tested
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 3:
Build Started http://104.196.14.100/job/kudu-gerrit/3056/
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4059/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 3177: // Defaults to true in old versions of Kudu.
> I thought this comment implied that we need only set obsolete_local if it's
I guess the issue is this code in 0.9.1:
todd@todd-ThinkPad-T540p:~/git/kudu$ git grep -A5 'has_local' 0.9.1
0.9.1:src/kudu/consensus/quorum_util.cc: if (!config.has_local()) {
0.9.1:src/kudu/consensus/quorum_util.cc- return Status::IllegalState(
0.9.1:src/kudu/consensus/quorum_util.cc- Substitute("RaftConfig must specify whether it is local. RaftConfig: ",
0.9.1:src/kudu/consensus/quorum_util.cc- config.ShortDebugString()));
0.9.1:src/kudu/consensus/quorum_util.cc- }
0.9.1:src/kudu/consensus/quorum_util.cc-
In other words, if we don't set the 'local' flag, it would default to 'true', but has_local() would still return false.
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4059
to look at the new patch set (#3).
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Actually support downgrade to version that has LocalConsensus
Commit 74210b2546df9fd5dec7bb926eeb524362d2da90 attempted to support
downgrade from 0.10.x to 0.9.x however there is specific validation in
VerifyRaftConfig() in Kudu 0.9.x that requires the optional field
"local" to be set (checked using has_local()). So we must not rely on
the default, rather, we must always set that field.
This patch was manually tested as follows:
A 3-way replicated table was created in 0.10.0-RC1 plus this patch,
using a single master and 3 tablet servers. The services were killed,
then restarted using 0.9.1 binaries.
Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 4 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/4059/3
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 1:
Build Started http://104.196.14.100/job/kudu-gerrit/2991/
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 2:
Build Started http://104.196.14.100/job/kudu-gerrit/3006/
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has submitted this change and it was merged.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Actually support downgrade to version that has LocalConsensus
Commit 74210b2546df9fd5dec7bb926eeb524362d2da90 attempted to support
downgrade from 0.10.x to 0.9.x however there is specific validation in
VerifyRaftConfig() in Kudu 0.9.x that requires the optional field
"local" to be set (checked using has_local()). So we must not rely on
the default, rather, we must always set that field.
This patch was manually tested as follows:
A 3-way replicated table was created in 0.10.0-RC1 plus this patch,
using a single master and 3 tablet servers. The services were killed,
then restarted using 0.9.1 binaries.
Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Reviewed-on: http://gerrit.cloudera.org:8080/4059
Tested-by: Kudu Jenkins
Reviewed-by: Dinesh Bhat <di...@cloudera.com>
Reviewed-by: Adar Dembo <ad...@cloudera.com>
---
M src/kudu/master/catalog_manager.cc
M src/kudu/master/sys_catalog.cc
2 files changed, 4 insertions(+), 2 deletions(-)
Approvals:
Dinesh Bhat: Looks good to me, but someone else must approve
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/4059
to look at the new patch set (#2).
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Actually support downgrade to version that has LocalConsensus
Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
---
M src/kudu/master/catalog_manager.cc
1 file changed, 4 insertions(+), 2 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/59/4059/2
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/4059/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 3177: // Defaults to true in old versions of Kudu.
> I guess the issue is this code in 0.9.1:
Oh, Adar yeah we should remove this comment. Todd you are correct about the CHECK in 0.9.1
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 4: Code-Review+1
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Kudu Jenkins (Code Review)" <ge...@cloudera.org>.
Kudu Jenkins has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 4:
Build Started http://104.196.14.100/job/kudu-gerrit/3068/
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 4: Code-Review+2
(1 comment)
Feel free to punt.
http://gerrit.cloudera.org:8080/#/c/4059/4/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
PS4, Line 3178: if (nreplicas == 1) {
: config->set_obsolete_local(true);
: } else {
: config->set_obsolete_local(false);
: }
Nit: maybe "config->set_obsolete_local(nreplicas == 1)"?
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] Actually support downgrade to version that has LocalConsensus
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: Actually support downgrade to version that has LocalConsensus
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/4059/1//COMMIT_MSG
Commit Message:
PS1, Line 10: there is a CHECK that was missed
: in the initial patch
Do you mean there's a CHECK that must be softened? If so, I don't see that change in this patch.
http://gerrit.cloudera.org:8080/#/c/4059/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 3177: // Defaults to true in old versions of Kudu.
I thought this comment implied that we need only set obsolete_local if it's going to be false, because if it's true, well, that's the default value in old versions anyway.
So why do we need to set it in both cases now? What changed in our understanding of how protobuf handles missing optional booleans? Can you update the comment so it's more useful?
--
To view, visit http://gerrit.cloudera.org:8080/4059
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Icded76c6400b4802581671e2e41efb74a47c5ec4
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes