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
Gerrit-Reviewer: Abhishek Rawat
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Comment-Date: Fri, 23 Oct 2020 16:41:58 +
Gerrit-HasComments: Yes
