[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-02 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-02 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


IMPALA-4899: Fix parquet table writer dictionary leak

Currently, in HdfsTableSink, OutputPartitions are added to the RuntimeState
object pool to be freed at the end of the query. However, for clustered inserts
into a partitioned table, the OutputPartitions are only used one at a time.
They can be immediately freed once done writing to that partition.

In addition, the HdfsParquetTableWriter's ColumnWriters are also added to
this object pool. These constitute a significant amount of memory, as they
contain the dictionaries for Parquet encoding.

This change makes HdfsParquetTableWriter's ColumnWriters use unique_ptrs so
that they are cleaned up when the HdfsParquetTableWriter is deleted. It also
uses a unique_ptr on the PartitionPair for the OutputPartition.

The table writers maintain a pointer to the OutputPartition. This remains a
raw pointer. This is safe, because OutputPartition has a scoped_ptr to the
table writer. The table writer will never outlive the OutputPartition.

Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Reviewed-on: http://gerrit.cloudera.org:8080/6181
Reviewed-by: Marcel Kornacker 
Tested-by: Impala Public Jenkins
---
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-table-writer.h
5 files changed, 26 insertions(+), 14 deletions(-)

Approvals:
  Marcel Kornacker: Looks good to me, approved
  Impala Public Jenkins: Verified



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-02 Thread Impala Public Jenkins (Code Review) 
Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 6:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/329/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-02 Thread Marcel Kornacker (Code Review) 
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Marcel Kornacker (Code Review) 
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Joe McDonnell (Code Review) 
Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6181

to look at the new patch set (#5).

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..

IMPALA-4899: Fix parquet table writer dictionary leak

Currently, in HdfsTableSink, OutputPartitions are added to the RuntimeState
object pool to be freed at the end of the query. However, for clustered inserts
into a partitioned table, the OutputPartitions are only used one at a time.
They can be immediately freed once done writing to that partition.

In addition, the HdfsParquetTableWriter's ColumnWriters are also added to
this object pool. These constitute a significant amount of memory, as they
contain the dictionaries for Parquet encoding.

This change makes HdfsParquetTableWriter's ColumnWriters use unique_ptrs so
that they are cleaned up when the HdfsParquetTableWriter is deleted. It also
uses a unique_ptr on the PartitionPair for the OutputPartition.

The table writers maintain a pointer to the OutputPartition. This remains a
raw pointer. This is safe, because OutputPartition has a scoped_ptr to the
table writer. The table writer will never outlive the OutputPartition.

Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
---
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-table-writer.h
5 files changed, 26 insertions(+), 14 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/6181/5
-- 
To view, visit http://gerrit.cloudera.org:8080/6181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Joe McDonnell (Code Review) 
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6181/4/be/src/exec/hdfs-table-sink.h
File be/src/exec/hdfs-table-sink.h:

Line 184:   typedef std::pair PartitionPair;
> to expand on that: your commit message explains the intention behind the un
Improved comments here and in hdfs-parquet-table-writer.h.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

Re: [Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Tim Armstrong 
The problem is that the memory is untracked so it won't be accounted
against the query.

You can look at process RSS or the process memory consumption on the /memz
to see the total memory consumed including untracked allocations.

The last time I ran into something like this I used a script from
stackoverflow to graph the memory consumption:
https://issues.cloudera.org/browse/IMPALA-2940
http://stackoverflow.com/questions/7998302/graphing-a-processs-memory-usage

On Wed, Mar 1, 2017 at 4:21 PM, Marcel Kornacker (Code Review) <
ger...@cloudera.org> wrote:

> Marcel Kornacker has posted comments on this change.
>
> Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
> ..
>
>
> Patch Set 4:
>
> (1 comment)
>
> http://gerrit.cloudera.org:8080/#/c/6181/4/be/src/exec/hdfs-table-sink.h
> File be/src/exec/hdfs-table-sink.h:
>
> Line 184:   typedef std::pair std::vector> PartitionPair;
> > please include updates to the class comments (of the affected classes)
> that
> to expand on that: your commit message explains the intention behind the
> unique_ptrs. however, that's not visible from the perspective of the next
> person looking at this code, so it's better to have this directly in the
> code.
>
>
> --
> To view, visit http://gerrit.cloudera.org:8080/6181
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
> Gerrit-MessageType: comment
> Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
> Gerrit-PatchSet: 4
> Gerrit-Project: Impala-ASF
> Gerrit-Branch: master
> Gerrit-Owner: Joe McDonnell 
> Gerrit-Reviewer: Joe McDonnell 
> Gerrit-Reviewer: Lars Volker 
> Gerrit-Reviewer: Marcel Kornacker 
> Gerrit-Reviewer: Tim Armstrong 
> Gerrit-HasComments: Yes
>

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Marcel Kornacker (Code Review) 
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6181/4/be/src/exec/hdfs-table-sink.h
File be/src/exec/hdfs-table-sink.h:

Line 184:   typedef std::pair PartitionPair;
> please include updates to the class comments (of the affected classes) that
to expand on that: your commit message explains the intention behind the 
unique_ptrs. however, that's not visible from the perspective of the next 
person looking at this code, so it's better to have this directly in the code.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Marcel Kornacker (Code Review) 
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6181/4/be/src/exec/hdfs-table-sink.h
File be/src/exec/hdfs-table-sink.h:

Line 184:   typedef std::pair PartitionPair;
please include updates to the class comments (of the affected classes) that 
briefly describe the memory management intentions. since we got this wrong last 
time around it appears to be subtle/non-obvious enough to warrant a description.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Joe McDonnell (Code Review) 
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 4:

> is there a way to ascertain the leak-free behavior in a test?

I don't see a difference in the runtime profile with this patch vs without. The 
way I have been testing is by hand. I do a clustered insert into a partitioned 
table and look at the memory usage in top. It is a very clear difference, but 
I'm not sure how to write a test around it. I tried setting a mem_limit and 
testing without my patch, but the mem_limit is not enforced.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Marcel Kornacker (Code Review) 
Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 4:

is there a way to ascertain the leak-free behavior in a test?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-03-01 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-02-28 Thread Joe McDonnell (Code Review) 
Joe McDonnell has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6181/3/be/src/exec/hdfs-table-sink.h
File be/src/exec/hdfs-table-sink.h:

Line 172:   const std::unique_ptr& output_partition,
> Just passing the raw pointer seems ok to me - I think you can do everything
You're right. Now that I think about it, there isn't much reason to use a const 
unique_ptr reference. I've switched it back to a raw pointer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-02-28 Thread Joe McDonnell (Code Review) 
Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/6181

to look at the new patch set (#4).

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..

IMPALA-4899: Fix parquet table writer dictionary leak

Currently, in HdfsTableSink, OutputPartitions are added to the RuntimeState
object pool to be freed at the end of the query. However, for clustered inserts
into a partitioned table, the OutputPartitions are only used one at a time.
They can be immediately freed once done writing to that partition.

In addition, the HdfsParquetTableWriter's ColumnWriters are also added to
this object pool. These constitute a significant amount of memory, as they
contain the dictionaries for Parquet encoding.

This change makes HdfsParquetTableWriter's ColumnWriters use unique_ptrs so
that they are cleaned up when the HdfsParquetTableWriter is deleted. It also
uses a unique_ptr on the PartitionPair for the OutputPartition.

The table writers maintain a pointer to the OutputPartition. This remains a
raw pointer. This is safe, because OutputPartition has a scoped_ptr to the
table writer. The table writer will never outlive the OutputPartition.

Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
---
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/hdfs-parquet-table-writer.h
M be/src/exec/hdfs-table-sink.cc
M be/src/exec/hdfs-table-sink.h
M be/src/exec/hdfs-table-writer.h
5 files changed, 16 insertions(+), 11 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/6181/4
-- 
To view, visit http://gerrit.cloudera.org:8080/6181
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong

[Impala-ASF-CR] IMPALA-4899: Fix parquet table writer dictionary leak

2017-02-28 Thread Tim Armstrong (Code Review) 
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4899: Fix parquet table writer dictionary leak
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6181/3/be/src/exec/hdfs-table-sink.h
File be/src/exec/hdfs-table-sink.h:

Line 172:   const std::unique_ptr& output_partition,
Just passing the raw pointer seems ok to me - I think you can do everything 
with the const unique_ptr that you can do with a raw pointer, so the type 
doesn't convey much additional info.

I don't feel that strongly about it though


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I06e354086ad24071d4fbf823f25f5df23933688f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes