Yida Wu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19087 )
Change subject: IMPALA-11631 Fix impala crashes in impala::TopNNode::Heap::Close() ...................................................................... Patch Set 4: (1 comment) http://gerrit.cloudera.org:8080/#/c/19087/3/be/src/exec/topn-node.cc File be/src/exec/topn-node.cc: http://gerrit.cloudera.org:8080/#/c/19087/3/be/src/exec/topn-node.cc@696 PS3, Line 696: eaps from partition_heap_ : // to rematerialized_heaps once all have been rematerialized. Otherwise, in case of > move() will set the original variable to null. So, as we are going through Thanks for pointing out this. Changed the comment. Discussed with Abhishek today, that there could also be a potential memory leak issue, because the Heap destructor doesn't call Close(), so the unique pointer moved to the local "rematerialized_heaps" may not release all the resources of the Heap automatically. After all, I keep the original change and adds a check for nullptr and a DCHECK in the closing process. So that the logic is to ensure to release all the Heaps in the TopNNode Close(). Maybe better to have the Close() in the Heap destructor, but it involves the structure change, seems more risky than the current change to fix the crash issue. Could be better to have a follow up later to review all these similar kinds of destructor design if the point makes sense. -- To view, visit http://gerrit.cloudera.org:8080/19087 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iaf45b6ef777f68e1843c076a935e4189acc6990b Gerrit-Change-Number: 19087 Gerrit-PatchSet: 4 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Joe McDonnell <[email protected]> Gerrit-Reviewer: Yida Wu <[email protected]> Gerrit-Comment-Date: Wed, 05 Oct 2022 03:50:51 +0000 Gerrit-HasComments: Yes
