You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Alexey Serbin (Code Review)" <ge...@cloudera.org> on 2017/02/10 04:19:58 UTC
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5964
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
Follow the common retry path scenario for Kudu C++ client
in case if ServiceUnavailable error received.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
1 file changed, 4 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/1
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5964/6/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
PS6, Line 241: if (s.ok() && resp->has_error()) {
: if (resp->error().code() == MasterErrorPB::NOT_THE_LEADER ||
: resp->error().code() == MasterErrorPB::CATALOG_MANAGER_NOT_INITIALIZED) {
: if (client->IsMultiMaster()) {
: KLOG_EVERY_N_SECS(INFO, 1) << "Determining the new leader Master and retrying...";
: WARN_NOT_OK(ConnectToCluster(client, deadline),
: "Unable to determine the new leader Master");
: }
: continue;
: } else {
: return StatusFromPB(resp->error().status());
: }
: }
> During the testing I found that at least the (s.ok() and && resp->has_error
I should have been more precise; it's L242-250 that I think are unreachable.
But yeah, we don't have to deal with this right now.
http://gerrit.cloudera.org:8080/#/c/5964/6/src/kudu/integration-tests/create-table-stress-test.cc
File src/kudu/integration-tests/create-table-stress-test.cc:
Line 337: SleepFor(MonoDelta::FromMilliseconds(1 + rand() % 10));
> 1) Since CreateTable() calls are started to back-off exponentially, that me
Okay, fair enough.
Line 351: if (s.IsTimedOut()) {
> We could, but I'm not sure it's possible to calculate that timeout to make
Hmm, OK. I don't feel strongly about it, and it's probably not important enough to get right.
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5964
to look at the new patch set (#7).
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well, for single- and multi-master
scenarios.
Besides, cleaned up invocation sites of SyncLeaderMasterRpc: moved
handling of ResponsePB::has_error() and converting the PB error into
Status into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/create-table-stress-test.cc
3 files changed, 56 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/7
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 11
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5964/6/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
PS6, Line 241: if (s.ok() && resp->has_error()) {
: if (resp->error().code() == MasterErrorPB::NOT_THE_LEADER ||
: resp->error().code() == MasterErrorPB::CATALOG_MANAGER_NOT_INITIALIZED) {
: if (client->IsMultiMaster()) {
: KLOG_EVERY_N_SECS(INFO, 1) << "Determining the new leader Master and retrying...";
: WARN_NOT_OK(ConnectToCluster(client, deadline),
: "Unable to determine the new leader Master");
: }
: continue;
: } else {
: return StatusFromPB(resp->error().status());
: }
: }
FWIW, I don't believe this case can be reached right now. I don't know if it was ever reachable, to be honest. AFAICT, the catalog manager responds with ServiceUnavailable or IllegalState if it also sets one of these errors. But, it's possible that this wasn't the case in the past; I don't know for sure.
http://gerrit.cloudera.org:8080/#/c/5964/6/src/kudu/integration-tests/create-table-stress-test.cc
File src/kudu/integration-tests/create-table-stress-test.cc:
Line 337: SleepFor(MonoDelta::FromMilliseconds(1 + rand() % 10));
Why this change?
Line 351: if (s.IsTimedOut()) {
Could we set the default admin operation timeout of the client real high and then avoid this case altogether?
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5964
to look at the new patch set (#6).
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well, for single- and multi-master
scenarios.
Besides, cleaned up invocation sites of SyncLeaderMasterRpc: moved
handling of ResponsePB::has_error() and converting the PB error into
Status into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/create-table-stress-test.cc
3 files changed, 56 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/6
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 12: Code-Review+2
Propagating +2 from Adar's review.
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 12
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 7: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 7
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Jean-Daniel Cryans (Code Review)" <ge...@cloudera.org>.
Jean-Daniel Cryans has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5964/5/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
Line 205: // The difference between NetworkError and ServiceUnavailable
Took me way too long to understand the difference here. Maybe say why it's different?
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5964/5/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
Line 205: // The difference between NetworkError and ServiceUnavailable
> Took me way too long to understand the difference here. Maybe say why it's
That's a good idea. In fact, I would like to understand the reason as well. :) Will try to initiate a conversation on the Slack channel.
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5964/1/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
Line 225: continue;
should we use the path below in case there's been a master failover?
Any way to provoke this in a unit test so it doesn't regress?
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5964
to look at the new patch set (#5).
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well, for single- and multi-master
scenarios.
Besides, cleaned up invocation sites of SyncLeaderMasterRpc: moved
handling of ResponsePB::has_error() and converting the PB error into
Status into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/create-table-stress-test.cc
3 files changed, 55 insertions(+), 71 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/5
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 5
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 6:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5964/6/src/kudu/integration-tests/create-table-stress-test.cc
File src/kudu/integration-tests/create-table-stress-test.cc:
Line 351: if (s.IsTimedOut()) {
> Hmm, OK. I don't feel strongly about it, and it's probably not important e
I'll add a TODO note and more information why it's done like this.
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 9: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 9
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 1:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5964/1/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
Line 225: continue;
> should we use the path below in case there's been a master failover?
I think so, but I was not sure about sending requests again if that was ServiceUnavailable response. I will dig more to understand if that makes sense.
Yes, I'll add the test.
The good news is that even in this half-baked condition the patch fixed all the tests which failed with my recent patch (not counting that deadlock on master shutdown).
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5964
to look at the new patch set (#4).
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well.
Also cleaned up invocation sites of SyncLeaderMasterRpc: moved handling
of ResponsePB::has_error() and converting the PB error into Status
into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
2 files changed, 46 insertions(+), 61 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/4
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5964
to look at the new patch set (#10).
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well, for single- and multi-master
scenarios.
Besides, cleaned up invocation sites of SyncLeaderMasterRpc: moved
handling of ResponsePB::has_error() and converting the PB error into
Status into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager.h
4 files changed, 68 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/10
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Adar Dembo, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5964
to look at the new patch set (#8).
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well, for single- and multi-master
scenarios.
Besides, cleaned up invocation sites of SyncLeaderMasterRpc: moved
handling of ResponsePB::has_error() and converting the PB error into
Status into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager.h
4 files changed, 68 insertions(+), 72 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/8
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 8
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well, for single- and multi-master
scenarios.
Besides, cleaned up invocation sites of SyncLeaderMasterRpc: moved
handling of ResponsePB::has_error() and converting the PB error into
Status into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Reviewed-on: http://gerrit.cloudera.org:8080/5964
Reviewed-by: Alexey Serbin <as...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
M src/kudu/integration-tests/create-table-stress-test.cc
M src/kudu/master/catalog_manager.h
4 files changed, 68 insertions(+), 72 deletions(-)
Approvals:
Alexey Serbin: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 13
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
Patch Set 6:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5964/6/src/kudu/client/client-internal.cc
File src/kudu/client/client-internal.cc:
PS6, Line 241: if (s.ok() && resp->has_error()) {
: if (resp->error().code() == MasterErrorPB::NOT_THE_LEADER ||
: resp->error().code() == MasterErrorPB::CATALOG_MANAGER_NOT_INITIALIZED) {
: if (client->IsMultiMaster()) {
: KLOG_EVERY_N_SECS(INFO, 1) << "Determining the new leader Master and retrying...";
: WARN_NOT_OK(ConnectToCluster(client, deadline),
: "Unable to determine the new leader Master");
: }
: continue;
: } else {
: return StatusFromPB(resp->error().status());
: }
: }
> FWIW, I don't believe this case can be reached right now. I don't know if i
During the testing I found that at least the (s.ok() and && resp->has_error()) was reachable.
I touched this piece of the code because:
1) I saw there would be an issue of non-retrying the call if that (code == NOT_THE_LEADER || code == CATALOG_MANAGER_NOT_INITIALIZED) sub-criterion is hit in case of single-master cluster
2) I wanted to consolidate that StatusFromPB() stuff for the scenarios when (s.ok() && resp->has_error()) is hit.
Do you mind if we address the reachability of the sub-criterion in a separate changelist?
http://gerrit.cloudera.org:8080/#/c/5964/6/src/kudu/integration-tests/create-table-stress-test.cc
File src/kudu/integration-tests/create-table-stress-test.cc:
Line 337: SleepFor(MonoDelta::FromMilliseconds(1 + rand() % 10));
> Why this change?
1) Since CreateTable() calls are started to back-off exponentially, that metadata reload activity makes the test long very-very long.
2) I though it might be better to employ some randomness here because this test is supposed to expose race conditions, if any (at least the main test comment says so).
Line 351: if (s.IsTimedOut()) {
> Could we set the default admin operation timeout of the client real high an
We could, but I'm not sure it's possible to calculate that timeout to make sure it works in every testing environment. The problem is with exponential-with-randomness back-off retry behavior.
I'm open for suggestions here -- if you think we can amend this scenario to keep the essence but remove the unpredictability, I can try to implement it.
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Jean-Daniel Cryans <jd...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client] retry operation in case of ServiceUnavailable
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/5964
to look at the new patch set (#2).
Change subject: [client] retry operation in case of ServiceUnavailable
......................................................................
[client] retry operation in case of ServiceUnavailable
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Follow the common retry path scenario for Kudu C++ client in case
of ServiceUnavailable error as well.
Also cleaned up invocation sites of SyncLeaderMasterRpc: moved handling
of ResponsePB::has_error() and converting the PB error into Status
into the code of the SyncLeaderMasterRpc method template.
This commit re-enables ClientTest::TestRetriesOnServiceUnavailable test
which passes with this fix.
Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
---
M src/kudu/client/client-internal.cc
M src/kudu/client/client-test.cc
2 files changed, 42 insertions(+), 58 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/64/5964/2
--
To view, visit http://gerrit.cloudera.org:8080/5964
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iae169ce1efe3950dfcb769594a1944d05e2ad8e7
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>