You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Henry Robinson (Code Review)" <ge...@cloudera.org> on 2016/10/31 22:25:12 UTC

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState 

Henry Robinson has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/4893

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

IMPALA-4410: Safer tear-down of RuntimeState

* Add RuntimeState::Close() which is guaranteed to release resources
  safely, rather than having PFE::Close() do the same piecemeal.
* Fix for crash where PFE::Prepare() fails before descriptor table is
  created.

Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Testing: Found by debug-actions, which has a reproducible test where
PFE::Prepare() fails. Manually tested on master.
---
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
3 files changed, 10 insertions(+), 4 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/4893/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4893
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4893

to look at the new patch set (#2).

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

IMPALA-4410: Safer tear-down of RuntimeState

* Add RuntimeState::Close() which is guaranteed to release resources
  safely, rather than having PFE::Close() do the same piecemeal.
* Fix for crash where PFE::Prepare() fails before descriptor table is
  created.
* Remove some dead code from TestEnv, and rename some methods for clarity.

Testing: Found by debug-actions, which has a reproducible test where
PFE::Prepare() fails. Manually tested on master.

Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
---
M be/src/exec/hash-table-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/service/fe-support.cc
A be/src/util/scope-exit-trigger.h
10 files changed, 82 insertions(+), 47 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/4893/2
-- 
To view, visit http://gerrit.cloudera.org:8080/4893
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.cc
File be/src/runtime/test-env.cc:

Line 109:   for (shared_ptr<RuntimeState>& runtime_state : runtime_states_) {
> Any thoughts?
The second alternative makes most sense to me since it preserves the original intent.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Sailesh Mukil (Code Review)" <ge...@cloudera.org>.
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 <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

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


Patch Set 4: Verified-1

Build failed: http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/531/

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4893

to look at the new patch set (#3).

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

IMPALA-4410: Safer tear-down of RuntimeState

* Add RuntimeState::Close() which is guaranteed to release resources
  safely, rather than having PFE::Close() do the same piecemeal.
* Fix for crash where PFE::Prepare() fails before descriptor table is
  created.
* Remove some dead code from TestEnv, and rename some methods for clarity.

Testing: Found by debug-actions, which has a reproducible test where
PFE::Prepare() fails. Manually tested on master.

Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
---
M be/src/exec/hash-table-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/service/fe-support.cc
A be/src/util/scope-exit-trigger.h
10 files changed, 95 insertions(+), 66 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/4893/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4893
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

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


Patch Set 5: Code-Review+2

(2 comments)

Carry +2, last patch didn't include the small test changes needed to make sure query IDs are unique.

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

Line 303:   Status CodegenScalarFns();
> let's call this ReleaseResources(), which is also going to be added to a nu
I think the destruction comment is pertinent - it conveys that this is a necessary step before destroying a runtime state.


http://gerrit.cloudera.org:8080/#/c/4893/4/be/src/runtime/test-env.h
File be/src/runtime/test-env.h:

Line 72:   /// Per-query states with associated block managers. Key is the integer query ID passed
> key?
Done


-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Sailesh Mukil, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4893

to look at the new patch set (#4).

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

IMPALA-4410: Safer tear-down of RuntimeState

* Add RuntimeState::Close() which is guaranteed to release resources
  safely, rather than having PFE::Close() do the same piecemeal.
* Fix for crash where PFE::Prepare() fails before descriptor table is
  created.
* Remove some dead code from TestEnv, and rename some methods for clarity.

Testing: Found by debug-actions, which has a reproducible test where
PFE::Prepare() fails. Manually tested on master.

Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
---
M be/src/exec/hash-table-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/service/fe-support.cc
A be/src/util/scope-exit-trigger.h
10 files changed, 95 insertions(+), 66 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/4893/4
-- 
To view, visit http://gerrit.cloudera.org:8080/4893
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

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


Patch Set 1:

(1 comment)

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()
> The ownership is actually really simple - TestEnv owns the RuntimeStates an
Oh, I see - no the problem is that TestEnv a) calls them query states and b) allocates and returns a naked pointer before finally wrapping it in a shared_ptr at the end of the method. I can make that change.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

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


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.cc
File be/src/runtime/test-env.cc:

Line 109:   for (shared_ptr<RuntimeState>& runtime_state : runtime_states_) {
> What do you propose - summing over the distinct query IDs? Or checking in C
Any thoughts?


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.cc
File be/src/runtime/test-env.cc:

Line 109:   for (shared_ptr<RuntimeState>& runtime_state : runtime_states_) {
This calculation is wrong unless you only created one RuntimeState per query (which was the original intent of the interface).


http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.h
File be/src/runtime/test-env.h:

PS2, Line 42: CreateRuntimeState
The old name was deliberate, since it's meant to provide a simple interface to set up the minimal set of query-wide objects for a test.

I.e. once there is actually a query-wide state, this should create it.

The new name seems ok, but there doesn't seem to be much point in changing it if it's going to get changed back after IMPALA-4014


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Marcel Kornacker (Code Review)" <ge...@cloudera.org>.
Marcel Kornacker has posted comments on this change.

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


Patch Set 4:

(2 comments)

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

Line 303:   /// Release resources and prepare this object for destruction.
let's call this ReleaseResources(), which is also going to be added to a number of other control structures, and remove comment about destruction.


http://gerrit.cloudera.org:8080/#/c/4893/4/be/src/runtime/test-env.h
File be/src/runtime/test-env.h:

Line 72:   /// Per-query states with associated block managers.
key?


-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

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


Patch Set 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()
> Agreed. TestEnv also creates RuntimeStates so we should call Close() there 
Fair point. I'm going to leave TestEnv for now - looking into that code it's a rat's nest, and we leak RuntimeStates everywhere as far as I can tell. Added it to fe-support - I added a utility to call Close() on scope exit, because there are so many paths out of that method...


Line 304:   ExecEnv::GetInstance()->thread_mgr()->UnregisterPool(resource_pool_);
> Any reason for changing the order in which you are closing now? (Unregister
No reason, don't think they're dependent.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(1 comment)

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()
> Fair point. I'm going to leave TestEnv for now - looking into that code it'
The ownership is actually really simple - TestEnv owns the RuntimeStates and destroys them in TestEnv::TearDownQueryStates(). So you could just close them in that function.

Maybe the red herring is that TestEnv::query_states_ uses shared_ptr. It could actually be switched to unique_ptr - the ownership is actually exclusive, the code was just written before C++11 support.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

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


Patch Set 4: Code-Review+2

Rebase.

-- 
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: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Hello Internal Jenkins, Sailesh Mukil, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

    http://gerrit.cloudera.org:8080/4893

to look at the new patch set (#5).

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

IMPALA-4410: Safer tear-down of RuntimeState

* Add RuntimeState::Close() which is guaranteed to release resources
  safely, rather than having PFE::Close() do the same piecemeal.
* Fix for crash where PFE::Prepare() fails before descriptor table is
  created.
* Remove some dead code from TestEnv, and rename some methods for clarity.

Testing: Found by debug-actions, which has a reproducible test where
PFE::Prepare() fails. Manually tested on master.

Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
---
M be/src/exec/hash-table-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/service/fe-support.cc
A be/src/util/scope-exit-trigger.h
10 files changed, 111 insertions(+), 72 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/93/4893/5
-- 
To view, visit http://gerrit.cloudera.org:8080/4893
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 3: Code-Review+2

-- 
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: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

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


Patch Set 5: Verified+1

-- 
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: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has submitted this change and it was merged.

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


IMPALA-4410: Safer tear-down of RuntimeState

* Add RuntimeState::Close() which is guaranteed to release resources
  safely, rather than having PFE::Close() do the same piecemeal.
* Fix for crash where PFE::Prepare() fails before descriptor table is
  created.
* Remove some dead code from TestEnv, and rename some methods for clarity.

Testing: Found by debug-actions, which has a reproducible test where
PFE::Prepare() fails. Manually tested on master.

Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Reviewed-on: http://gerrit.cloudera.org:8080/4893
Tested-by: Internal Jenkins
Reviewed-by: Henry Robinson <he...@cloudera.com>
---
M be/src/exec/hash-table-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/service/fe-support.cc
A be/src/util/scope-exit-trigger.h
10 files changed, 111 insertions(+), 72 deletions(-)

Approvals:
  Henry Robinson: Looks good to me, approved
  Internal Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ie416e4d57240142bf685385299b749c3a6792c45
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker <ma...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

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


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.cc
File be/src/runtime/test-env.cc:

Line 109:   for (shared_ptr<RuntimeState>& runtime_state : runtime_states_) {
> This calculation is wrong unless you only created one RuntimeState per quer
What do you propose - summing over the distinct query IDs? Or checking in Create<whatever>State() that the query_id hasn't been used before? It looks like at most if not all callsites it's supposed to be called with a unique id, so perhaps that's an invariant we should enforce in the Create...() method.


http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.h
File be/src/runtime/test-env.h:

PS2, Line 42: CreateRuntimeState
> The old name was deliberate, since it's meant to provide a simple interface
I can change it back; it seemed almost obfuscatory here because all it did was create a RuntimeState with some associated structures and QueryState has another meaning already. Maybe CreatePerQueryState() will be clearer.


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

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


Patch Set 1:

(1 comment)

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:
Agreed. TestEnv also creates RuntimeStates so we should call Close() there too.


-- 
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 <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4410: Safer tear-down of RuntimeState

Posted by "Henry Robinson (Code Review)" <ge...@cloudera.org>.
Henry Robinson has posted comments on this change.

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


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.cc
File be/src/runtime/test-env.cc:

Line 109:   for (shared_ptr<RuntimeState>& runtime_state : runtime_states_) {
> The second alternative makes most sense to me since it preserves the origin
Done


http://gerrit.cloudera.org:8080/#/c/4893/2/be/src/runtime/test-env.h
File be/src/runtime/test-env.h:

PS2, Line 42: CreateRuntimeState
> I can change it back; it seemed almost obfuscatory here because all it did 
Done


-- 
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: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Henry Robinson <he...@cloudera.com>
Gerrit-Reviewer: Sailesh Mukil <sa...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes