Michael Ho has posted comments on this change. Change subject: IMPALA-5558: Reopen stale client connection ......................................................................
Patch Set 7: (4 comments) http://gerrit.cloudera.org:8080/#/c/7284/7/be/src/runtime/client-cache.h File be/src/runtime/client-cache.h: Line 203: client_is_unrecoverable_(false) { > Not for this patch, but this would be a good place to inject a fault, i.e. Done http://gerrit.cloudera.org:8080/#/c/7284/7/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS7, Line 347: if (i < 2) SleepForMs(100); > why? Sleep for 100ms between retry in case of intermittent bad network connection. This is a best effort retry. PS7, Line 356: connect_success > What is 'connect_success' trying to achieve? It looks like it wants to keep It's trying to preserve the old behavior of the code. The header comment states that it returns true if cancellation was attempted. http://gerrit.cloudera.org:8080/#/c/7284/7/be/src/runtime/query-state.cc File be/src/runtime/query-state.cc: PS7, Line 237: !client_status.ok() > No, this is still outside the loop, so we're still at least trying 3 times The new patch will retry 3 times as Sailesh said. It seems safer to revert to the old behavior of not cancelling the fragment instances if we fail. -- 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: 7 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
