You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Dan Burkert (Code Review)" <ge...@cloudera.org> on 2017/11/08 02:43:16 UTC
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Dan Burkert has uploaded this change for review. ( http://gerrit.cloudera.org:8080/8494
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
5 files changed, 173 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/1
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 1
Gerrit-Owner: Dan Burkert <da...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
Patch Set 8:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc@219
PS8, Line 219: IsEndOfFile
> hrm, should we replace this with NetworkError? EOF is pretty odd to get on
I think EOF is a normal error message to get when the receiving side of the connection has been closed. This error is originating in Thrift as a TTransportException with type TTransportException::END_OF_FILE.
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client.cc@85
PS8, Line 85: IOError
> maybe NetworkError is better here
Done
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.h@60
PS8, Line 60: meastore
> typo
Done
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.cc@144
PS8, Line 144: Continue
> we use the word Resume for other minicluster daemons
Done
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Mon, 29 Jan 2018 22:58:40 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
Patch Set 11: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 31 Jan 2018 21:02:45 +0000
Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#10).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 192 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/10
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 10
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#8).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 192 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/8
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
Patch Set 7:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@51
PS7, Line 51: the
to
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@84
PS7, Line 84: explicit
No longer needed now that this is multi-arg?
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@85
PS7, Line 85: MonoDelta send_timeout = MonoDelta::FromSeconds(60),
Would be more ergonomic to create an HmsClientOptions struct and stuff these (with documentation) in there.
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@111
PS7, Line 111: "-p", std::to_string(port_),
Was this change made so that you could use Start() to restart and have it use the same port as before? Might want to document that behavior.
Also, how do we prevent test flakiness caused by another application binding to our previously bound ephemeral port?
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@131
PS7, Line 131: WARN_NOT_OK
Why not RETURN_NOT_OK?
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@139
PS7, Line 139: KUDU
You can drop this prefix. In Continue() too.
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 7
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Thu, 04 Jan 2018 00:36:29 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#5).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
5 files changed, 178 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/5
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 5
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#11).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 191 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/11
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 11
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#4).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
5 files changed, 178 insertions(+), 21 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/4
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 4
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
Patch Set 8:
(4 comments)
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc@219
PS8, Line 219: IsEndOfFile
hrm, should we replace this with NetworkError? EOF is pretty odd to get on a network call
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client.cc
File src/kudu/hms/hms_client.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client.cc@85
PS8, Line 85: IOError
maybe NetworkError is better here
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.h
File src/kudu/hms/mini_hms.h:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.h@60
PS8, Line 60: meastore
typo
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/mini_hms.cc@144
PS8, Line 144: Continue
we use the word Resume for other minicluster daemons
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 21:13:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
Patch Set 8:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h
File src/kudu/hms/hms_client.h:
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@51
PS7, Line 51: eco
> to
Done
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@84
PS7, Line 84: static c
> No longer needed now that this is multi-arg?
Apparently it is, since the other args are optional. I tried removing it, but got a tidy warning.
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/hms_client.h@85
PS7, Line 85: static const char* const kKuduMasterAddrsKey;
> Would be more ergonomic to create an HmsClientOptions struct and stuff thes
Done
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc
File src/kudu/hms/mini_hms.cc:
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@111
PS7, Line 111: "-p", std::to_string(port_),
> Might want to document that behavior.
Done.
> Also, how do we prevent test flakiness caused by another application binding to our previously bound ephemeral port?
It's quite unlikely that someone else would bind to the ephemeral port in between HMS restarts; the OS won't immediately reuse the port.
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@131
PS7, Line 131: RETURN_NOT_
> Why not RETURN_NOT_OK?
Done
http://gerrit.cloudera.org:8080/#/c/8494/7/src/kudu/hms/mini_hms.cc@139
PS7, Line 139: RETU
> You can drop this prefix. In Continue() too.
Done
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 24 Jan 2018 19:49:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#2).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
5 files changed, 175 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/2
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 2
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#9).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 192 insertions(+), 23 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/9
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 9
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Kudu Jenkins,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/8494
to look at the new patch set (#3).
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
5 files changed, 175 insertions(+), 18 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/94/8494/3
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 3
Gerrit-Owner: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
KUDU-2191 (4/n): HMS Thrift client fault handling
This commit improves the new HMS Thrift client's ability to handle
faults. In particular:
- The client now uses send, receive, and connect timeouts so that a
non-responsive HMS instance will not block the client indefinitely.
- The Thrift logging callback is hooked up to glog so that we get proper
log messages from Thrift.
- The HmsClient class and method docs include information about behavior
when errors are encountered.
In the part 2 review, Todd also brought up the prospect of creating a
wrapper Thrift socket or transport to inject slow log warning messages
automatically. I've held off doing this for now, because I haven't been
able to figure out a way to do that which can associate the slowness
with higher-level operations like 'create database', as opposed to
lower-level like 'socket write'. I've made sure to apply the slow
warning calls uniformly across the HmsClient methods, and I don't think
it will be too onerous to keep them consistent in the future.
Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Reviewed-on: http://gerrit.cloudera.org:8080/8494
Tested-by: Kudu Jenkins
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M src/kudu/hms/hms_client-test.cc
M src/kudu/hms/hms_client.cc
M src/kudu/hms/hms_client.h
M src/kudu/hms/mini_hms.cc
M src/kudu/hms/mini_hms.h
M src/kudu/mini-cluster/external_mini_cluster-test.cc
6 files changed, 191 insertions(+), 23 deletions(-)
Approvals:
Kudu Jenkins: Verified
Todd Lipcon: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 12
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Todd Lipcon (Code Review)" <ge...@cloudera.org>.
Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc@219
PS8, Line 219: IsEndOfFile
> I think EOF is a normal error message to get when the receiving side of the
It's inconsistent with KRPC behavior though, which is confusing. I think it's better to be internally consistent across our RPC mechanisms even if Thrift reports it as EOF.
I also think, as a user/operator, if I saw a message like "end of file" I'd start looking for issues on the file system rather than understanding that a network connection was broken. (never mind the implementation detail that sockets are sorta-kinda-files in unix)
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 31 Jan 2018 02:02:30 +0000
Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change. ( http://gerrit.cloudera.org:8080/8494 )
Change subject: KUDU-2191 (4/n): HMS Thrift client fault handling
......................................................................
Patch Set 8:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc
File src/kudu/hms/hms_client-test.cc:
http://gerrit.cloudera.org:8080/#/c/8494/8/src/kudu/hms/hms_client-test.cc@219
PS8, Line 219: IsEndOfFile
> It's inconsistent with KRPC behavior though, which is confusing. I think it
Done
--
To view, visit http://gerrit.cloudera.org:8080/8494
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic48bbb833bbae39b383ae239054b9710da3c746d
Gerrit-Change-Number: 8494
Gerrit-PatchSet: 8
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Tidy Bot
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Comment-Date: Wed, 31 Jan 2018 20:08:22 +0000
Gerrit-HasComments: Yes