impala-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Sailesh Mukil (Code Review)" <ger...@cloudera.org>
Subject [Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState
Date Mon, 31 Oct 2016 22:57:27 GMT
Sailesh Mukil has posted comments on this change.

Change subject: IMPALA-4410: Safer tear-down of RuntimeState
......................................................................


Patch Set 1: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4893/1/be/src/runtime/runtime-state.cc
File be/src/runtime/runtime-state.cc:

PS1, Line 300: RuntimeState::Close()
Nothing very pressing but wanted to bring this up:

Having this generic Close() call for the RuntimeState means one would expect it to be called
at the end of every RuntimeState's lifetime.

However, we have this RuntimeState in fe-support that looks like a straggler now, since we
don't call Close() on it (we don't need to as well, because none of the things being closed
here are even set up for that object).


Line 304:   ExecEnv::GetInstance()->thread_mgr()->UnregisterPool(resource_pool_);
Any reason for changing the order in which you are closing now? (UnregisterPool() was after
UnregisterReaderContexts() before this patch).

If yes, might be good to briefly comment why.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <henry@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sailesh@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message