You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Matthew Jacobs (Code Review)" <ge...@cloudera.org> on 2016/11/19 21:11:53 UTC

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink 

Matthew Jacobs has uploaded a new change for review.

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

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................

IMPALA-4478: Initial Kudu client mem tracking for sink

The Kudu client allocates memory which is not tracked by
Impala. There are several sources, but the most significant
is the memory allocated by the KuduSession on the write
path. This can be >100MB, so it is important to track to
avoid OOM.

Moving forward, we should have a better way to track Kudu
client memory, but for now we must at least handle this
potentially problematic case.

This changes the KuduTableSink to consume 200MB which should
be enough for the 100MB write mutation buffer as well as
100MB worth of errors buffered in the client before Impala
takes ownership of them (and deletes them). This is left as
a flag because it may turn out to be too high for some users
and too low for others. When we have better support from
Kudu (including KUDU-1752), we should simplify this.

TODO: Handle DML w/ small or known resource requirements
(e.g. VALUES specified or query has LIMIT) specially to
avoid over-consumption.

Testing: Have verified acceptable behavior in the stress
test with a simple workload containing DML statements of
moderate cardinality.

Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
---
M be/src/exec/kudu-table-sink.cc
1 file changed, 32 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/5152/1
-- 
To view, visit http://gerrit.cloudera.org:8080/5152
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5152/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS1, Line 34: 100 * 1024 * 1024
how about pulling this into a #define and using it here and below, to make it impossible to miss the connection.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Tim Armstrong (Code Review)" <ge...@cloudera.org>.
Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5152/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

Line 49:     " required (bytes) for a KuduTableSink. This flag is subject to change or removal.");
Can you add something here that indicates that it is typically 2 * kudu_mutation_buffer_size. Otherwise there's no info in the help text that the two options are correlated.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Hello Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................

IMPALA-4478: Initial Kudu client mem tracking for sink

The Kudu client allocates memory which is not tracked by
Impala. There are several sources, but the most significant
is the memory allocated by the KuduSession on the write
path. This can be >100MB, so it is important to track to
avoid OOM.

Moving forward, we should have a better way to track Kudu
client memory, but for now we must at least handle this
potentially problematic case.

This changes the KuduTableSink to consume 200MB which should
be enough for the 100MB write mutation buffer as well as
100MB worth of errors buffered in the client before Impala
takes ownership of them (and deletes them). This is left as
a flag because it may turn out to be too high for some users
and too low for others. When we have better support from
Kudu (including KUDU-1752), we should simplify this.

TODO: Handle DML w/ small or known resource requirements
(e.g. VALUES specified or query has LIMIT) specially to
avoid over-consumption.

Testing: Have verified acceptable behavior in the stress
test with a simple workload containing DML statements of
moderate cardinality.

Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
---
M be/src/exec/kudu-table-sink.cc
1 file changed, 37 insertions(+), 3 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/52/5152/2
-- 
To view, visit http://gerrit.cloudera.org:8080/5152
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5152/1/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS1, Line 34: 100 * 1024 * 1024
> how about pulling this into a #define and using it here and below, to make 
Done


Line 49:     " required (bytes) for a KuduTableSink. This flag is subject to change or removal.");
> Can you add something here that indicates that it is typically 2 * kudu_mut
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Dan Hecht (Code Review)" <ge...@cloudera.org>.
Dan Hecht has posted comments on this change.

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................


Patch Set 2: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5152/2/be/src/exec/kudu-table-sink.cc
File be/src/exec/kudu-table-sink.cc:

PS2, Line 34: define
oops, I guess this should really be "static const int".


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Internal Jenkins (Code Review)" <ge...@cloudera.org>.
Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................


IMPALA-4478: Initial Kudu client mem tracking for sink

The Kudu client allocates memory which is not tracked by
Impala. There are several sources, but the most significant
is the memory allocated by the KuduSession on the write
path. This can be >100MB, so it is important to track to
avoid OOM.

Moving forward, we should have a better way to track Kudu
client memory, but for now we must at least handle this
potentially problematic case.

This changes the KuduTableSink to consume 200MB which should
be enough for the 100MB write mutation buffer as well as
100MB worth of errors buffered in the client before Impala
takes ownership of them (and deletes them). This is left as
a flag because it may turn out to be too high for some users
and too low for others. When we have better support from
Kudu (including KUDU-1752), we should simplify this.

TODO: Handle DML w/ small or known resource requirements
(e.g. VALUES specified or query has LIMIT) specially to
avoid over-consumption.

Testing: Have verified acceptable behavior in the stress
test with a simple workload containing DML statements of
moderate cardinality.

Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Reviewed-on: http://gerrit.cloudera.org:8080/5152
Reviewed-by: Matthew Jacobs <mj...@cloudera.com>
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Internal Jenkins
---
M be/src/exec/kudu-table-sink.cc
1 file changed, 37 insertions(+), 3 deletions(-)

Approvals:
  Matthew Jacobs: Looks good to me, approved
  Internal Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-4478: Initial Kudu client mem tracking for sink

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4478: Initial Kudu client mem tracking for sink
......................................................................


Patch Set 2: Code-Review+2

carrying +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I47f17a81e4362ab490019382fedc66c25f07080a
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-HasComments: No