Tim Armstrong has posted comments on this change. ( http://gerrit.cloudera.org:8080/8303 )
Change subject: IMPALA-1575: Part 1: eagerly release query exec resources ...................................................................... Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc File be/src/runtime/coordinator.cc: http://gerrit.cloudera.org:8080/#/c/8303/10/be/src/runtime/coordinator.cc@1095 PS10, Line 1095: query_state_ != nullptr) > But if the query_state_ were null, then we wouldn't release the resources, That would require that query_state_ was not initialised but we acquired a resource refcount. With the code as it is now, it's trivial to convince ourselves that it's not possible. I can remove the null check but my concern was that it's a brittle assumption that there are no failure paths where a reference to the query_state_ is not obtained. I had to look at the implementation of two functions across two files to convince myself that it was true. It seems more like to me that code would move around to invalidate the second assumption than the first. -- To view, visit http://gerrit.cloudera.org:8080/8303 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I41ff374b0403f10a145f7fee9b3145953ee32341 Gerrit-Change-Number: 8303 Gerrit-PatchSet: 10 Gerrit-Owner: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Joe McDonnell <joemcdonn...@cloudera.com> Gerrit-Reviewer: Sailesh Mukil <sail...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-Comment-Date: Thu, 26 Oct 2017 22:14:27 +0000 Gerrit-HasComments: Yes
