[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

Talked with Michael offline. We agreed that it would be good to defer the 
teardown of RuntimeState::fragment_instance_tracker_ as well.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

Thanks!

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

I wrote up a short page on the wiki per Henry's suggestion: 
https://cwiki.apache.org/confluence/display/IMPALA/Resource+Management+Best+Practices+in+Impala

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

Any more comments?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

OK. Thanks for the clarification. This kind of implicit rule is always 
confusing to me.

I suppose this change makes sense to avoid the crash in release build if the 
MemTracker is somehow referenced after it has been unlinked from its parent but 
before it's destroyed. On the other hand, this makes me question if there are 
undesirable side-effect to allow such references to the MemTracker object after 
it has been unlinked from the hierarchy.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

It would be great if someone could write the best-practice resource management 
pattern down on the wiki. It always seems to cause confusion in code reviews.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

Matt's understanding matches mine - releasing resources in 
Close()/ReleaseResources() but only destroying the objects during query 
teardown.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

> Isn't the general direction we are moving towards to not rely on
 > destructor to free control states ? Instead, we want to free them
 > explicitly in functions like Close() or ReleaseResources().

I thought we want to release any state in the destructor, but not necessarily 
to free the memory of the control struct objects.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1:

Isn't the general direction we are moving towards to not rely on destructor to 
free control states ? Instead, we want to free them explicitly in functions 
like Close() or ReleaseResources().

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..


Patch Set 1: Code-Review+1

Seems like the right thing to do (i.e. control state cleanup happening in the 
destructor), regardless of whether or not this fixes the crash in IMPALA-5715.

Let's see if Michael wants to take a look, but otherwise seems like a safe 
thing to commit.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

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

Change subject: IMPALA-5715: (potential mitigation) don't destroy MemTracker 
during Close()
..

IMPALA-5715: (potential mitigation) don't destroy MemTracker during Close()

One potential candidate for the bad MemTracker IMPALA-5715 is one owned
by a join build sink. I haven't found a clear root cause, but we can
reduce the probability of bugs like this by deferring teardown of the
MemTrackers.

Testing:
Ran a private core test run.

Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
---
M be/src/exec/data-sink.cc
M be/src/runtime/mem-tracker.cc
2 files changed, 3 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I205abb0076d1ffd08cb93c0f1671c8b81e7fba0f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong