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/12 00:20:51 UTC
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Alexey Serbin has uploaded a new change for review.
http://gerrit.cloudera.org:8080/5974
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
3 files changed, 126 insertions(+), 42 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/1
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 7:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5974/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
Line 4721: MonoDelta::FromSeconds(2),
> To avoid flakiness, maybe set this one a little higher, or set the client t
Done
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
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/5974
to look at the new patch set (#2).
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
3 files changed, 127 insertions(+), 41 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/2
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] [client-test] ServiceUnavailable retry scenario
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/5974
to look at the new patch set (#4).
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.h
2 files changed, 64 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/4
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Todd Lipcon <to...@apache.org>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5974/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
Line 4675: SleepFor(MonoDelta::FromSeconds(2));
I wonder whether this will prove flaky. With only two seconds it's conceivable for weird scheduling and/or load to delay the CreateTable() RPC until after the usurper releases the lock.
Maybe the usurper can wait on a countdown latch here instead, and when CreateTable() times out below, it can release the latch so that the usurper is unblocked and releases the lock?
Oh, I see you discussed that with Todd. What if, after the usurper thread is unblocked from the latch, it sleeps for another second or two? That should prevent the scenario I described above while still allowing the usurper to hold the lock as the second CreateTable() attempt begins.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
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/5974
to look at the new patch set (#5).
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.h
2 files changed, 58 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/5
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Todd Lipcon <to...@apache.org>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 8:
> > If you've got the time, may be good to loop the new test in TSAN
> > mode.
>
> Will do, thanks.
Actually, I already did, but not many iterations. Will try more and use dist-test for that as well.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 8:
> If you've got the time, may be good to loop the new test in TSAN
> mode.
http://dist-test.cloudera.org//job?job_id=aserbin.1487116091.9191
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client-test] ServiceUnavailable retry scenario
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/5974
to look at the new patch set (#7).
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.h
2 files changed, 87 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/7
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 8:
> If you've got the time, may be good to loop the new test in TSAN
> mode.
Will do, thanks.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has submitted this change and it was merged.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
Reviewed-on: http://gerrit.cloudera.org:8080/5974
Reviewed-by: Adar Dembo <ad...@cloudera.com>
Tested-by: Kudu Jenkins
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.h
2 files changed, 87 insertions(+), 0 deletions(-)
Approvals:
Adar Dembo: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 2:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 153: Status MiniMaster::StartDistributedMasterOnPorts(uint16_t rpc_port, uint16_t web_port,
> ok, will do.
I just tried to keep the order of the methods in the implementation file to be consistent with the order of their declaration.
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/master/mini_master.h
File src/kudu/master/mini_master.h:
PS2, Line 59: // The Init() and StartInitted() methods below are used to separate
: // execution of Master::Init() and Master::StartAsync() methods,
: // so it's possible to tinker with the master object in between.
: Status Init(uint16_t rpc_port = 0);
: Status StartInitted();
> can you clarify in the docs here that the combination of these two, in this
Done
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 5:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5974/5/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
Line 4675: SleepFor(MonoDelta::FromSeconds(2));
> I wonder whether this will prove flaky. With only two seconds it's conceiva
Good catch. Yes, I agree -- in this modified scenario timings have changed and there is more root for flakiness. I'll update this as suggested. Actually, I'm thinking to make it a parameterized test.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5974/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS3, Line 4669: shared_ptr<KuduClient> client;
: ASSERT_OK(KuduClientBuilder()
: .add_master_server_addr(bound_rpc.ToString())
: .Build(&client));
Why can't we reuse client_?
Line 4683: ASSERT_OK(cluster_->mini_master()->StartAlreadyInitted());
Why is it important to split up Init() and Start()? Don't you still wind up with ServiceUnavailable errors if you usurp the election lock belonging to a fully initialized catalog manager?
The Init/Start split is a fair amount of mini_master complexity for just one test. It also makes our eventual goal of unifying mini/external cluster start/stop routines harder. If you really need to slow down the catalog manager startup sequence, perhaps you can inject some latency instead? Check out util/fault_injection.h.
Line 4691: gscoped_ptr<KuduTableCreator> table_creator(client->NewTableCreator());
unique_ptr
PS3, Line 4706: gscoped_ptr
unique_ptr.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
Gerrit-PatchSet: 3
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-test] ServiceUnavailable retry scenario
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/5974
to look at the new patch set (#8).
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.h
2 files changed, 87 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/8
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 2:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
Line 4665: MonoDelta block_interval) {
to make this less timing-dependent, why not take in a CountdownLatch to wait on? then the test can just countdown the latch after the first timeout?
PS2, Line 4688: thread(bind(&BlockCatalogManager,
: catalog,
: MonoDelta::FromSeconds(2))).detach();
:
might be fewer overall lines just to use a lambda here
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 153: Status MiniMaster::StartDistributedMasterOnPorts(uint16_t rpc_port, uint16_t web_port,
this is a little hard to review because you moved the function while changing it - can't see side by side diff of what changed. can you put it back in its old position?
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/master/mini_master.h
File src/kudu/master/mini_master.h:
PS2, Line 59: // The Init() and StartInitted() methods below are used to separate
: // execution of Master::Init() and Master::StartAsync() methods,
: // so it's possible to tinker with the master object in between.
: Status Init(uint16_t rpc_port = 0);
: Status StartInitted();
can you clarify in the docs here that the combination of these two, in this order, are equivalent to the normal "Start()' call?
maybe 'StartAlreadyInitted()' would be a clearer name as well
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS2, Line 4688: thread(bind(&BlockCatalogManager,
: catalog,
: MonoDelta::FromSeconds(2))).detach();
:
> Do you mean lambda for the function to be called by the thread? I tried th
yea, just:
thread([&]() {
master::CatalogManager::ScopedLeaderSharedLock l(catalog);
SleepFor(MonoDelta::FromSeconds(2));
}).detach()
that didn't work?
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
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/5974
to look at the new patch set (#6).
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/catalog_manager.h
2 files changed, 79 insertions(+), 0 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/6
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 8: Code-Review+2
If you've got the time, may be good to loop the new test in TSAN mode.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No
[kudu-CR] [client-test] ServiceUnavailable retry scenario
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/5974
to look at the new patch set (#3).
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
[client-test] ServiceUnavailable retry scenario
KUDU-1871 Kudu C++ client does not retry on ServiceUnavailable errors
Added test to verify behavior of the Kudu C++ client in case of
ServiceUnavailable error returned from master server. The test is
currently disabled because it fails. The follow-up commit will
add the fix and enable the test.
Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
---
M src/kudu/client/client-test.cc
M src/kudu/master/mini_master.cc
M src/kudu/master/mini_master.h
3 files changed, 111 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/74/5974/3
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
Gerrit-PatchSet: 3
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>
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 7: Code-Review+2
(1 comment)
Clever test parameterization.
http://gerrit.cloudera.org:8080/#/c/5974/7/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
Line 4721: MonoDelta::FromSeconds(2),
To avoid flakiness, maybe set this one a little higher, or set the client timeout a little lower, or both? I get that you don't want this to be too high because it draws out the test runtime, but the delta between the two seems too low to me.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 2:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS2, Line 4688: thread(bind(&BlockCatalogManager,
: catalog,
: MonoDelta::FromSeconds(2))).detach();
:
> yea, just:
With small variation: I used variable for the lambda. That did not work :)
Just using the in-lined code should work, I think.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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>
Gerrit-HasComments: Yes
[kudu-CR] [client-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 3:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/5974/3/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
PS3, Line 4669: shared_ptr<KuduClient> client;
: ASSERT_OK(KuduClientBuilder()
: .add_master_server_addr(bound_rpc.ToString())
: .Build(&client));
> Why can't we reuse client_?
Good question: I think we can re-use the existing one. Here I tried some other path in the very beginning which required a separate client.
Line 4683: ASSERT_OK(cluster_->mini_master()->StartAlreadyInitted());
> Why is it important to split up Init() and Start()? Don't you still wind up
My original intention was to simulate the conditions under which the non-retriable issue was observed. After some consideration I think that's not crucial.
Usurping the lock via ScopedLeaderSharedLock() of fully initialized cluster does not help: it's necessary to acquire the lock in an exclusive manner if using that scenario. ok, I'll make this test acquiring the leader_lock_ in an exclusive manner.
Line 4691: gscoped_ptr<KuduTableCreator> table_creator(client->NewTableCreator());
> unique_ptr
Done
PS3, Line 4706: gscoped_ptr
> unique_ptr.
Done
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
Gerrit-PatchSet: 3
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-test] ServiceUnavailable retry scenario
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change.
Change subject: [client-test] ServiceUnavailable retry scenario
......................................................................
Patch Set 2:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/client/client-test.cc
File src/kudu/client/client-test.cc:
Line 4665: MonoDelta block_interval) {
> to make this less timing-dependent, why not take in a CountdownLatch to wai
I want the second request to start when the catalog manager is still blocked -- want to see it ends as a success.
PS2, Line 4688: thread(bind(&BlockCatalogManager,
: catalog,
: MonoDelta::FromSeconds(2))).detach();
:
> might be fewer overall lines just to use a lambda here
Do you mean lambda for the function to be called by the thread? I tried that but it didn't work -- compilation error. Probably, should have used 'static' or something like that for the lambda variable.
http://gerrit.cloudera.org:8080/#/c/5974/2/src/kudu/master/mini_master.cc
File src/kudu/master/mini_master.cc:
Line 153: Status MiniMaster::StartDistributedMasterOnPorts(uint16_t rpc_port, uint16_t web_port,
> this is a little hard to review because you moved the function while changi
ok, will do.
--
To view, visit http://gerrit.cloudera.org:8080/5974
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ief82add7b36c1f3875ea77e7f7503f6fb552838c
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>
Gerrit-HasComments: Yes