Todd Lipcon has posted comments on this change. Change subject: rpc: avoid crashing if the server generates an invalid RPC response ......................................................................
Patch Set 2: (5 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? actually SerializeResponseBuffer() no longer can return anything but OK, so I made it void Line 142: > Path seems to have not touch method "SerializeResponseTo" . That CHECK happened because 'SerializeResponseBuffer' was returning a bad status (and thus not filling in the response buffer). Now, SerializeResponseBuffer always fills in some data, so we won't have an empty buffer. 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 fie yea, will add comment. 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. we're not super consistent, but I think they should start with a lower case, because we typically concatenate them with ':' between. Like: "NetworkError: got an RPC issue: invalid parameter: missing fields: foo". which looks better than capitalizing the first letter after a ': ' 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; yea, in the case that you send an request with missing fields, now you still get a RemoteError, but it's not an ERROR_APPLICATION, so we can't run ExtractRemoteError() on it to pull out the remote-bootstrap-specific one -- 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: Todd Lipcon <[email protected]> Gerrit-Reviewer: song bruce zhang <[email protected]> Gerrit-HasComments: Yes
