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 2017/05/10 17:20:20 UTC

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM 

Matthew Jacobs has uploaded a new change for review.

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................

IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

On jenkins.impala.io gerrit-verify-dryrun jobs,
free-pool-test started running out of memory after updating
Kudu to a newer version which can have slightly higher
memory requirements (Kudu will reject writes when mem is 80%
full rather than 60% full). free-pool-test can allocate up
to 12gb, and the VMs have a CommitLimit of only 16gb.

While larger VMs could be used, or the minicluster could be
tuned further, it doesn't seem like free-pool-test needs to
be testing such large allocations.

Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
---
M be/src/runtime/free-pool-test.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

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

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 2:

I measured the max rss w/ this change to be 88mb, down from 14gb is what I observed with the old code.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Hello Michael Ho,

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

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

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................

IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

On jenkins.impala.io gerrit-verify-dryrun jobs,
free-pool-test started running out of memory after updating
Kudu to a newer version which can have slightly higher
memory requirements (Kudu will reject writes when mem is 80%
full rather than 60% full). free-pool-test can allocate up
to 12gb, and the VMs have a CommitLimit of only 16gb.

While larger VMs could be used, or the minicluster could be
tuned further, free-pool-test can also be modified to
reduce the actual RSS usage. Instead of memsetting the
entire allocation in the test, we only scribble the first
byte. This reduces the max RSS from 14gb to 88mb in some
local tests.

Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
---
M be/src/runtime/free-pool-test.cc
1 file changed, 3 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/42/6842/3
-- 
To view, visit http://gerrit.cloudera.org:8080/6842
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 1:

Looking at git history, the large allocations were added here, so let's be sure we're not losing coverage we think we need. Please talk to Michael.

commit ed5ec6772fd24ad28901bc68f120c59597439cf2
Author: Michael Ho <kw...@cloudera.com>
Date:   Thu Jun 30 15:15:27 2016 -0700

    IMPALA-1619: Support 64-bit allocations.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 3:

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 3: Code-Review+2

> I measured the max rss w/ this change to be 88mb, down from 14gb is
 > what I observed with the old code.

it might still fail with vm_overcommit = 2 (https://www.kernel.org/doc/Documentation/vm/overcommit-accounting) but I agree this is the best way out.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 2:

Looks good. I will let Dan +2 it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6842/1/be/src/runtime/free-pool-test.cc
File be/src/runtime/free-pool-test.cc:

PS1, Line 138: memset(p1, 1, size);
> I wonder if we will be less prone to the problem if we don't actually memse
That's probably true, depending on vm_overcommit setting. But given that we are seeing the OOM killer, yes, this would probably be an alternate workaround.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/6842/1/be/src/runtime/free-pool-test.cc
File be/src/runtime/free-pool-test.cc:

PS1, Line 138: memset(p1, 1, size);
> That's probably true, depending on vm_overcommit setting. But given that we
Should be sufficient to replace it with *p1 = 1;


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

Posted by "Matthew Jacobs (Code Review)" <ge...@cloudera.org>.
Matthew Jacobs has uploaded a new patch set (#2).

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................

IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

On jenkins.impala.io gerrit-verify-dryrun jobs,
free-pool-test started running out of memory after updating
Kudu to a newer version which can have slightly higher
memory requirements (Kudu will reject writes when mem is 80%
full rather than 60% full). free-pool-test can allocate up
to 12gb, and the VMs have a CommitLimit of only 16gb.

While larger VMs could be used, or the minicluster could be
tuned further, it doesn't seem like free-pool-test needs to
be testing such large allocations.

Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
---
M be/src/runtime/free-pool-test.cc
1 file changed, 3 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6842/1/be/src/runtime/free-pool-test.cc
File be/src/runtime/free-pool-test.cc:

PS1, Line 132: 1LL
Will 2LL be too much ? I was hoping we could exercise the code for 64-bit allocation size.


PS1, Line 138: memset(p1, 1, size);
I wonder if we will be less prone to the problem if we don't actually memset() the entire chunk of memory.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/6842/1/be/src/runtime/free-pool-test.cc
File be/src/runtime/free-pool-test.cc:

PS1, Line 132: 1LL
> Will 2LL be too much ? I was hoping we could exercise the code for 64-bit a
we can keep this as is


PS1, Line 138: memset(p1, 1, size);
> Should be sufficient to replace it with *p1 = 1;
Good idea, this works well


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 3:

fwiw the jenkins nodes (both public and private) appear to have vm_overcommit 0

ubuntu@ip-172-31-7-2:~$ cat /proc/sys/vm/overcommit_memory 
0

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
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: Michael Ho <kw...@cloudera.com>
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

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

Change subject: IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM
......................................................................


IMPALA-5297: Reduce free-pool-test mem requirement to avoid OOM

On jenkins.impala.io gerrit-verify-dryrun jobs,
free-pool-test started running out of memory after updating
Kudu to a newer version which can have slightly higher
memory requirements (Kudu will reject writes when mem is 80%
full rather than 60% full). free-pool-test can allocate up
to 12gb, and the VMs have a CommitLimit of only 16gb.

While larger VMs could be used, or the minicluster could be
tuned further, free-pool-test can also be modified to
reduce the actual RSS usage. Instead of memsetting the
entire allocation in the test, we only scribble the first
byte. This reduces the max RSS from 14gb to 88mb in some
local tests.

Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
Reviewed-on: http://gerrit.cloudera.org:8080/6842
Reviewed-by: Dan Hecht <dh...@cloudera.com>
Tested-by: Impala Public Jenkins
---
M be/src/runtime/free-pool-test.cc
1 file changed, 3 insertions(+), 2 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I31f03e7a4d5d237a1183277c988f85a992396a43
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Dan Hecht <dh...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs <mj...@cloudera.com>
Gerrit-Reviewer: Michael Ho <kw...@cloudera.com>