impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Tim Armstrong (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-1575: Part 1: eagerly release query exec resources
Date Thu, 26 Oct 2017 22:14:27 GMT
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 <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Joe McDonnell <joemcdonnell@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Comment-Date: Thu, 26 Oct 2017 22:14:27 +0000
Gerrit-HasComments: Yes

Mime
  • Unnamed multipart/alternative (inline, 8-Bit, 0 bytes)
View raw message