[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: No

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Adar Dembo (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Jean-Daniel Cryans (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Alexey Serbin 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Todd Lipcon (Code Review)]

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 
Gerrit-Reviewer: Adar Dembo 
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[kudu-CR] [client] retry operation in case of ServiceUnavailable

[Alexey Serbin (Code Review)]

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