Abhishek Rawat has posted comments on this change. ( http://gerrit.cloudera.org:8080/16638 )
Change subject: IMPALA-10102 Fix Impalad crashses when writting a parquet file with large rows. ...................................................................... Patch Set 1: (6 comments) http://gerrit.cloudera.org:8080/#/c/16638/1//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/16638/1//COMMIT_MSG@7 PS1, Line 7: IMPALA-10102 Fix Impalad crashses when writting a parquet file with typo: writting -> writing crashses -> crashes You could probably reword the title to: "Impalad crashes when writing a parquet file with large rows" http://gerrit.cloudera.org:8080/#/c/16638/1//COMMIT_MSG@10 PS1, Line 10: The crash happens due to accessing a null pointer. The reason for the : accessing the null pointer is because there is no enough space left : in the memory pool when the system is suffering memory scarcity, : so the pool returns a null pointer when memory allocation failed, : however the code doesn't verify it and then lead to a crash This can probably be reworded and shortened: "The crash happens when trying to dereference a null pointer returned from a failed memory allocation." http://gerrit.cloudera.org:8080/#/c/16638/1//COMMIT_MSG@16 PS1, Line 16: The change is to use TryAllocate instead of Allocate and if it : returns null pointer then returns an error status. The change is : added to two places which are supposed to cause the crash. Could be reworded as: "TryAllocate is used instead of Allocate and null check is added for the large memory allocations such as buffer for dictionary page and compressed dictionary page. The memory allocation is most likely to fail for these large allocations when memory is scarce." http://gerrit.cloudera.org:8080/#/c/16638/1//COMMIT_MSG@20 PS1, Line 20: The change fixes the crash issue, however in practice, there : still be an OOM issue which leads the process being killed by the : OS. The change doesn't fix the OOM issue, users need to have a : proper configuration on the mem_limit(start-up option) to avoid the : OOM happens. Could be reworded as follows: "This change fixes the crash in this particular code path, however in practice, there could still be an OOM issue which could lead to the process getting killed by the OS. The change doesn't fix the OOM issue, users need to configure the mem_limit (start-up option) properly to avoid the OOM crash." http://gerrit.cloudera.org:8080/#/c/16638/1/be/src/exec/parquet/hdfs-parquet-table-writer.cc File be/src/exec/parquet/hdfs-parquet-table-writer.cc: http://gerrit.cloudera.org:8080/#/c/16638/1/be/src/exec/parquet/hdfs-parquet-table-writer.cc@764 PS1, Line 764: RETURN_STATUS_IF_ALLOC_NULL(dict_buffer, header.uncompressed_page_size); I think you could probably use MemTracker's MemLimitExceeded status for memory allocation failure. This is consistent with how we handle allocation failures in other areas of the code. And probably good to use the UNLIKELY attribute so that compiler can optimize the branch. An example from other areas of the code: https://github.com/apache/impala/blob/master/be/src/util/decompress.cc#L93 http://gerrit.cloudera.org:8080/#/c/16638/1/be/src/exec/parquet/hdfs-parquet-table-writer.cc@773 PS1, Line 773: RETURN_STATUS_IF_ALLOC_NULL(compressed_data, max_compressed_size); Same comment as above. -- To view, visit http://gerrit.cloudera.org:8080/16638 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0dee474cceb0c370278d290eb900c05769b23dec Gerrit-Change-Number: 16638 Gerrit-PatchSet: 1 Gerrit-Owner: Yida Wu <[email protected]> Gerrit-Reviewer: Abhishek Rawat <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Comment-Date: Fri, 23 Oct 2020 16:41:58 +0000 Gerrit-HasComments: Yes
