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 <tarmstr...@cloudera.com> Gerrit-Reviewer: Dan Hecht <dhe...@cloudera.com> Gerrit-Reviewer: Thomas Tauber-Marshall <tmarsh...@cloudera.com> Gerrit-Reviewer: Tim Armstrong <tarmstr...@cloudera.com> Gerrit-HasComments: Yes
