You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2018/01/17 01:47:27 UTC
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Hello Alexey Serbin, Dan Burkert,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/9041
to review the following change.
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/tserver/tablet_server-test.cc
6 files changed, 82 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9041/1
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 1
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 5: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 18 Jan 2018 00:37:20 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9041/4/src/kudu/server/rpc_server.cc
File src/kudu/server/rpc_server.cc:
http://gerrit.cloudera.org:8080/#/c/9041/4/src/kudu/server/rpc_server.cc@213
PS4, Line 213: return Status::IllegalState(Substitute("bad state: $0", server_state_));
I think this might be better as ServiceUnavailable, since it's expected to become available shortly. I think we also are more lenient about retries in the client when ServiceUnavailable is encountered.
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 18 Jan 2018 00:21:25 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9041
to look at the new patch set (#5).
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/server/webserver.cc
M src/kudu/tserver/tablet_server-test.cc
7 files changed, 90 insertions(+), 17 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9041/5
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 5
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 2:
(5 comments)
lgtm, just a few nits
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc@522
PS1, Line 522: rpc
nit: RPC
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc@525
PS1, Line 525: rpc
nit: RPC
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@211
PS2, Line 211: controller.Reset();
nit: maybe, just move RpcController under the scope of the 'while() {}' loop below?
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@215
PS2, Line 215: CHECK(resp.has_status());
nit: since there are many iterations of calling the GetStatus() method, maybe it's cleaner to move the 'resp' under the scope of the 'while() {}' loop to start with a clean state every iteration?
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@230
PS2, Line 230: ASSERT_OK(mini_server_->Restart());
nit: consider adding some scope-related mechanics to join the thread if this assert fails.
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 17 Jan 2018 02:21:05 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9041
to look at the new patch set (#3).
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/tserver/tablet_server-test.cc
6 files changed, 87 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9041/3
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9041
to look at the new patch set (#4).
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/tserver/tablet_server-test.cc
6 files changed, 88 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9041/4
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 2:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc
File src/kudu/server/server_base.cc:
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc@522
PS1, Line 522: rpc
> nit: RPC
Done
http://gerrit.cloudera.org:8080/#/c/9041/1/src/kudu/server/server_base.cc@525
PS1, Line 525: rpc
> nit: RPC
Done
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@211
PS2, Line 211: controller.Reset();
> nit: maybe, just move RpcController under the scope of the 'while() {}' loo
I appreciate the suggestion, but stylistically I prefer the "declare req, resp, and controller together" idiom we commonly use.
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@215
PS2, Line 215: CHECK(resp.has_status());
> nit: since there are many iterations of calling the GetStatus() method, may
See above.
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@230
PS2, Line 230: ASSERT_OK(mini_server_->Restart());
> nit: consider adding some scope-related mechanics to join the thread if thi
Done
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 17 Jan 2018 22:37:16 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Reviewed-on: http://gerrit.cloudera.org:8080/9041
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/server/webserver.cc
M src/kudu/tserver/tablet_server-test.cc
7 files changed, 90 insertions(+), 17 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 6
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Hello Alexey Serbin, Dan Burkert, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/9041
to look at the new patch set (#2).
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
KUDU-2148: do not crash on GetStatus during server startup
The fix is straight-forward but there are backwards compatibility
implications for old clients who trigger the race: because they're not aware
of the new 'error' field in the protobuf, they'll just see the incomplete
'status' field. To be more specific, 'node_instance' will be set,
'version_info' won't be, and 'bound_{rpc,http}_addresses' may or may not be
set depending on exactly when the race was hit.
An alternative would be to fail the RPC itself in lieu of adding the 'error'
field. That would ensure that old clients register a failure, though it
comes at the expense of not providing rich error information for new
clients, which is why I went with the above solution instead.
Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
---
M src/kudu/server/generic_service.cc
M src/kudu/server/rpc_server.cc
M src/kudu/server/server_base.cc
M src/kudu/server/server_base.h
M src/kudu/server/server_base.proto
M src/kudu/tserver/tablet_server-test.cc
6 files changed, 82 insertions(+), 15 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/41/9041/2
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 2
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@215
PS2, Line 215: (s.ok()) {
> Could you then at least add resp.Clear() to pair controller.Reset() ?
Done
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 17 Jan 2018 23:14:47 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc
File src/kudu/tserver/tablet_server-test.cc:
http://gerrit.cloudera.org:8080/#/c/9041/2/src/kudu/tserver/tablet_server-test.cc@215
PS2, Line 215: // These two fields are g
> See above.
Could you then at least add resp.Clear() to pair controller.Reset() ?
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 3
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Wed, 17 Jan 2018 23:00:42 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 18 Jan 2018 00:02:37 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2148: do not crash on GetStatus during server startup
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/9041 )
Change subject: KUDU-2148: do not crash on GetStatus during server startup
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/9041/4/src/kudu/server/rpc_server.cc
File src/kudu/server/rpc_server.cc:
http://gerrit.cloudera.org:8080/#/c/9041/4/src/kudu/server/rpc_server.cc@213
PS4, Line 213: return Status::IllegalState(Substitute("bad state: $0", server_state_));
> I think this might be better as ServiceUnavailable, since it's expected to
I was trying to be consistent with Webserver::GetBoundAddresses, which returns IllegalState. But I suppose I can update both.
--
To view, visit http://gerrit.cloudera.org:8080/9041
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3b997999d8a1228244e0be0ea61705f2c12e3426
Gerrit-Change-Number: 9041
Gerrit-PatchSet: 4
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Comment-Date: Thu, 18 Jan 2018 00:28:04 +0000
Gerrit-HasComments: Yes