Dan Hecht has posted comments on this change. Change subject: IMPALA-5558/IMPALA-5576: Reopen stale client connection ......................................................................
Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/7284/8/be/src/rpc/thrift-util.cc File be/src/rpc/thrift-util.cc: PS8, Line 198: TTransportException::END_OF_FILE && : strstr(e.what(), "No more data to read. how do we know that this means the connection was reset? is it because thrift would never get into this state otherwise? http://gerrit.cloudera.org:8080/#/c/7284/8/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS8, Line 350: dummy explain why this can be ignored. PS8, Line 362: connect_success i don't understand that. connect_success can be true if the previous client_status was ok yet the last rpc_status was not ok. why do we want to return true only in that case? why not just always return true? the return value is suppose to be if "cancellation was attempted", and it's only used for a log message. http://gerrit.cloudera.org:8080/#/c/7284/8/be/src/runtime/coordinator-backend-state.h File be/src/runtime/coordinator-backend-state.h: PS8, Line 138: message ReportExecStatus RPC messages to be more specific. http://gerrit.cloudera.org:8080/#/c/7284/8/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: Line 225: // Try to send the RPC 3 times before failing. Sleep for 100ms between retries. it'd be good to explain that retry is always safe for this rpc because the coordinator handles that explicitly. PS8, Line 236: res.status now that we try o get the connection inside the loop, this could be uninitialized, no? or does it get initialized to ok in TReportExecStatusResult constructor? Line 244: Cancel(); this Cancel() still results in IMPALA-5576, no? I'm not saying we should do anything more, though. at least we'll have tried a few times before this. -- To view, visit http://gerrit.cloudera.org:8080/7284 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4d722c8ad3bf0e78e89887b6cb286c69ca61b8f5 Gerrit-PatchSet: 8 Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-Owner: Michael Ho <[email protected]> Gerrit-Reviewer: Dan Hecht <[email protected]> Gerrit-Reviewer: Juan Yu <[email protected]> Gerrit-Reviewer: Lars Volker <[email protected]> Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Michael Ho <[email protected]> Gerrit-Reviewer: Mostafa Mokhtar <[email protected]> Gerrit-Reviewer: Sailesh Mukil <[email protected]> Gerrit-HasComments: Yes
