Adar Dembo has posted comments on this change. Change subject: rpc: avoid crashing if the server generates an invalid RPC response ......................................................................
Patch Set 2: (4 comments) http://gerrit.cloudera.org:8080/#/c/2914/2/src/kudu/rpc/inbound_call.cc File src/kudu/rpc/inbound_call.cc: Line 128: LOG(FATAL) << "Unable to serialize response for " Won't LOG(FATAL) crash the server? Don't you want LOG(ERROR) here? http://gerrit.cloudera.org:8080/#/c/2914/2/src/kudu/rpc/rpc-test-base.h File src/kudu/rpc/rpc-test-base.h: Line 239: context->RespondSuccess(); So the idea is that resp->response hasn't been set, and it's a required field, and that tickles the changes you made to RPC processing? http://gerrit.cloudera.org:8080/#/c/2914/2/src/kudu/rpc/service_if.cc File src/kudu/rpc/service_if.cc: Line 51: string err = Substitute("invalid parameter for call $0: missing fields: $1", Nit: I thought Status messages are supposed to begin with capital letters. That's the feedback I've been giving Dan in some recent code reviews. http://gerrit.cloudera.org:8080/#/c/2914/2/src/kudu/tserver/remote_bootstrap_service-test.cc File src/kudu/tserver/remote_bootstrap_service-test.cc: Line 158: controller->error_response()->code() != ErrorStatusPB::ERROR_APPLICATION) { Not obvious what this change was for. Could you explain? In gerrit is fine; I'm not asking for a code comment. -- To view, visit http://gerrit.cloudera.org:8080/2914 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia7845388b1f520158e554548d668836d40b10ca6 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: song bruce zhang <[email protected]> Gerrit-HasComments: Yes
