impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Marcel Kornacker (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4890/5143: Coordinator race involving TearDown()
Date Thu, 25 May 2017 15:33:09 GMT
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4890/5143: Coordinator race involving TearDown()
......................................................................


Patch Set 1:

(1 comment)

> (2 comments)
 > 
 > Where does ReleaseResources() get called for DML queries? It used
 > to be in ClientRequestState::Done(), I think, but I don't see where
 > it gets called now (it must be, because tests are passing).

This is now done implicitly as part of Cancel(). UnregisterQuery() calls CRS::Cancel (which
in turn calls Coordinator::Cancel) before it calls Done().

http://gerrit.cloudera.org:8080/#/c/6897/1/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

PS1, Line 113: if (query_state_ != nullptr) {
             :     ExecEnv::GetInstance()->query_exec_mgr()->ReleaseQueryState(query_state_);
             :   }
> I thought that in general we wanted to move away from work done in d'tors?
we want to move away from release resources in d'tors, and from tearing down control structures
prematurely (as was done in teardown()). the coordinator now has access to the query state
until the coordinator itself goes away.


-- 
To view, visit http://gerrit.cloudera.org:8080/6897
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I457a6424a0255c137336c4bc01a6e7ed830d18c7
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker <marcel@cloudera.com>
Gerrit-Reviewer: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <marcel@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message