[Impala-ASF-CR] IMPALA-7931: fix executor shutdown races

["Thomas Marshall (Code Review)" <ge...@cloudera.org>]

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 <ta...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-Reviewer: Thomas Marshall <th...@cmu.edu>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 04 Jan 2019 19:39:49 +0000
Gerrit-HasComments: Yes