Sailesh Mukil has posted comments on this change. Change subject: IMPALA-5558/IMPALA-5576: Reopen stale client connection ......................................................................
Patch Set 10: (4 comments) Have one final question about the sleeping holding the coordinator lock. Will +1 after we come to a conclusion on that. http://gerrit.cloudera.org:8080/#/c/7284/10/be/src/runtime/coordinator-backend-state.cc File be/src/runtime/coordinator-backend-state.cc: PS10, Line 248: if (instance_stats->done_) continue; It would be good to mention this fix in the commit message too. i.e. previously we would drop some other finstances statuses if we received duplicate 'done' statuses from the same finstance(s). PS10, Line 353: SleepForMs(100); Just wondering if it's a good idea to have the coordinator thread go to sleep. It may hold up a lot of work even though it's just for 300ms, since it takes the coordinator lock_ before calling this in Coordinator::Cancel(). Is there a risk in leaving it as it was before, i.e. try to cancel without a sleep? PS10, Line 356: status_.MergeStatus(client_status); Isn't this redundant if you're adding its message as a detail below in L361 anyway? PS10, Line 365: status_.MergeStatus(rpc_status); Same here -- 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: 10 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
