You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dinesh Bhat (Code Review)" <ge...@cloudera.org> on 2016/12/05 05:05:51 UTC
[kudu-CR] KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Hello Mike Percy, Adar Dembo,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/5357
to review the following change.
Change subject: KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
......................................................................
KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Master continuously retries DeleteTablet RPC for few non-retriable
error codes like WRONG_SERVER_UUID or TABLET_NOT_RUNNING both of
which indicate that tablet ceases to exist on that tserver.
Such errors can be deemed fatal and DeleteTablet need not be retried.
Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
---
M src/kudu/master/catalog_manager.cc
1 file changed, 22 insertions(+), 11 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/57/5357/1
--
To view, visit http://gerrit.cloudera.org:8080/5357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
[kudu-CR] KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
......................................................................
Patch Set 1:
(1 comment)
Looks good. As we discussed, please try to build tests for this using tserver-side metrics to verify that DeleteTablet() RPCs aren't received once the master has figured out that it should stop.
If anyone else has suggestions on how Dinesh could test this, please chime in!
http://gerrit.cloudera.org:8080/#/c/5357/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 2692: virtual string LogFailure(const string& reason, const Status& status) {
Why is this method virtual? I don't see any overrides anywhere.
--
To view, visit http://gerrit.cloudera.org:8080/5357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Posted by "Dinesh Bhat (Code Review)" <ge...@cloudera.org>.
Dinesh Bhat has posted comments on this change.
Change subject: KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
......................................................................
Patch Set 3:
> (1 comment)
>
> Looks good. As we discussed, please try to build tests for this
> using tserver-side metrics to verify that DeleteTablet() RPCs
> aren't received once the master has figured out that it should
> stop.
>
> If anyone else has suggestions on how Dinesh could test this,
> please chime in!
Hi Adar/Mike, TFTR here, things changed slightly over past couple of days around other patches related to this, hence putting this on hold at the moment. I will be addressing comments on these once I figure out the other patches related to these error codes are in good shape, so please bear with me.
--
To view, visit http://gerrit.cloudera.org:8080/5357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
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: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5357/1//COMMIT_MSG
Commit Message:
Line 11: which indicate that tablet ceases to exist on that tserver.
> TABLET_NOT_RUNNING could mean that the tablet is in the process of bootstra
I think we should consider renaming TABLET_NOT_RUNNING or being very precise about what it means via docs / comments
--
To view, visit http://gerrit.cloudera.org:8080/5357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5357/1//COMMIT_MSG
Commit Message:
Line 11: which indicate that tablet ceases to exist on that tserver.
TABLET_NOT_RUNNING could mean that the tablet is in the process of bootstrapping, in which case we _should_ keep retrying, so that when the tablet finishes bootstrapping it gets properly deleted, no?
Or do we properly delete it anyway because when it finishes bootstrapping, it reports some kind of change in a tablet report, resulting in a new DeleteTablet being sent?
--
To view, visit http://gerrit.cloudera.org:8080/5357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
......................................................................
Patch Set 1:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5357/1/src/kudu/master/catalog_manager.cc
File src/kudu/master/catalog_manager.cc:
Line 2693: return Substitute("TS $0 T $1: delete failed for tablet $2: $3",
This is some pretty old code, I think. The error message prefix is a bit weird now, I think it would be better to stick with the "T <tablet> P <peer>" prefix which is standard elsewhere. Maybe something like:
T 00000 P <master-uuid>: Delete of tablet <tablet-id> from peer <peer-uuid> failed due to <reason>: <error-string> (<TabletServerErrorPB::Code_Name()>)
Line 2723: case TabletServerErrorPB::TABLET_NOT_RUNNING:
Is the meaning of this code documented somewhere?
--
To view, visit http://gerrit.cloudera.org:8080/5357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-HasComments: Yes
[kudu-CR] KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
Posted by "Mike Percy (Code Review)" <ge...@cloudera.org>.
Mike Percy has posted comments on this change.
Change subject: KUDU-1608: Catalog manager can stop retrying DeleteTablet upon fatal errors
......................................................................
Patch Set 3:
Seems like a bad rebase on this patch.
--
To view, visit http://gerrit.cloudera.org:8080/5357
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id45f07667b6e62ce4814acfdf931dea2af4332d1
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <di...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No