Thomas Marshall has posted comments on this change. ( http://gerrit.cloudera.org:8080/12082 )
Change subject: IMPALA-7931: fix executor shutdown races ...................................................................... Patch Set 11: (6 comments) http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/common/status.h File be/src/common/status.h: http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/common/status.h@122 PS11, Line 122: If 'silent' is false, a traceback is logged : /// by the constructor, otherwise nothing is logged. I find this comment sort of confusing here, since all of these constructors always use silent=false. Maybe 'Since silent is false, a traceback is logged by the constructor.', and/or maybe modify the comment above the constructors that take the 'silent' parameter below. http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/runtime/coordinator.cc@832 PS11, Line 832: // Build set from vector so that runtime of this function is O(candidates.size()). Doesn't the loop below make this function O(backend_states_.size()), where potentially backend_states_.size() >> candidates.size()? http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/service/cancellation-work.h File be/src/service/cancellation-work.h: http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/service/cancellation-work.h@31 PS11, Line 31: SERVER_TERMINATED This makes me think that the server itself is what was terminated. Maybe TERMINATED_BY_SERVER? Or even just TERMINATED? Not a big deal if you prefer to leave as is, I see that you've gone through several different not-quite-perfect options for this. http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/service/cancellation-work.h@38 PS11, Line 38: be typo http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/service/cancellation-work.h@46 PS11, Line 46: Expiry ServerTerminated()? http://gerrit.cloudera.org:8080/#/c/12082/11/be/src/service/cancellation-work.h@90 PS11, Line 90: BACKEND_FAILED SERVER_TERMINATED -- To view, visit http://gerrit.cloudera.org:8080/12082 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7c1a80304cb6695d228aca8314e2231727ab1998 Gerrit-Change-Number: 12082 Gerrit-PatchSet: 11 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Impala Public Jenkins <impala-public-jenk...@cloudera.com> Gerrit-Reviewer: Michael Ho <k...@cloudera.com> Gerrit-Reviewer: Thomas Marshall <thomasmarsh...@cmu.edu> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Fri, 04 Jan 2019 19:39:49 +0000 Gerrit-HasComments: Yes