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-4748: crash in TmpFileMgr when hitting process mem limit
Date Thu, 19 Jan 2017 00:13:16 GMT
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4748: crash in TmpFileMgr when hitting process mem limit
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5683/3/be/src/runtime/tmp-file-mgr.cc
File be/src/runtime/tmp-file-mgr.cc:

Line 531:     is_cancelled_ = true;
> i don't understand why this line is needed (or the right thing), even with 
Clarified a bit more. It avoids the DCHECK in ~WriteHandle(). I want to leave the DCHECK there
since it can detect if the WriteHandle wasn't cleaned up properly by DestroyWriteHandle()
or CancelWriteAndDestroyData().

The key thing is that if we fail here, the WriteHandle reference is not returned to the caller
of Write(), so we won't go through the normal destruction path of DestroyWriteHandle() or
CancelWriteAndDestroyData().


Line 545:     write_in_flight_ = false;
> and then why does this case not need to set is_cancelled if the other one n
If we fail here, the WriteHandle was returned to the FileGroup client by Write(). So it will
be destroyed in the normal way by calling DestroyWriteHandle() or CancelWriteAndDestroyData().


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib0a624212bc17f7824e6d14ad143c0d5894206f8
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dhecht@cloudera.com>
Gerrit-Reviewer: Thomas Tauber-Marshall <tmarshall@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <tarmstrong@cloudera.com>
Gerrit-HasComments: Yes

Mime
View raw message