[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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--- 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 Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 31 Jan 2018 21:02:45 + Gerrit-HasComments: No
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 31 Jan 2018 20:08:22 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 31 Jan 2018 02:02:30 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 29 Jan 2018 22:58:40 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 24 Jan 2018 21:13:44 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Dan Burkert Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Wed, 24 Jan 2018 19:49:03 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Thu, 04 Jan 2018 00:36:29 + Gerrit-HasComments: Yes
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[kudu-CR] KUDU-2191 (4/n): HMS Thrift client fault handling
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 BurkertGerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot
[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