You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Todd Lipcon (Code Review)" <ge...@cloudera.org> on 2016/08/24 23:43:24 UTC

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map 

Hello Dan Burkert, Adar Dembo,

I'd like you to do a code review.  Please visit

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

to review the following change.

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................

java: fix leak of TabletClient objects in client2tablets map

After running YCSB for a week, most of the clients had hit OOMEs. Some
heap dump analysis showed that the client2tablets map had hundreds of
thousands of leaked clients.

It seems that we were neglecting to remove the client from the
client2tablets map upon a disconnect. This fixes the issue and adds a
regression test which reproduced the bug.

Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
4 files changed, 52 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/4119/1
-- 
To view, visit http://gerrit.cloudera.org:8080/4119
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


java: fix leak of TabletClient objects in client2tablets map

After running YCSB for a week, most of the clients had hit OOMEs. Some
heap dump analysis showed that the client2tablets map had hundreds of
thousands of leaked clients.

It seems that we were neglecting to remove the client from the
client2tablets map upon a disconnect. This fixes the issue and adds a
regression test which reproduced the bug.

This patch was worked on by Todd Lipcon and David Alves.

Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Reviewed-on: http://gerrit.cloudera.org:8080/4119
Tested-by: David Ribeiro Alves <dr...@apache.org>
Reviewed-by: Todd Lipcon <to...@apache.org>
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
4 files changed, 72 insertions(+), 5 deletions(-)

Approvals:
  David Ribeiro Alves: Verified
  Todd Lipcon: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 2:

ignore the re-push, accidentally rebased

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4119/1/java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
File java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java:

Line 45:   protected static final Logger LOG = LoggerFactory.getLogger(BaseKuduTest.class);
Is this necessary?  It doesn't look like you actually use it. I think we typically create loggers in individual tests classes if they need it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 3: Code-Review+2

(1 comment)

lgtm.

http://gerrit.cloudera.org:8080/#/c/4119/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:

Line 1803:       // Make a copy so we don't need to synchronize on it while iterating.
I really have no idea what's going on with the locking here... but it's not your fault.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

Posted by "David Ribeiro Alves (Code Review)" <ge...@cloudera.org>.
Hello Kudu Jenkins,

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

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

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................

java: fix leak of TabletClient objects in client2tablets map

After running YCSB for a week, most of the clients had hit OOMEs. Some
heap dump analysis showed that the client2tablets map had hundreds of
thousands of leaked clients.

It seems that we were neglecting to remove the client from the
client2tablets map upon a disconnect. This fixes the issue and adds a
regression test which reproduced the bug.

This patch was worked on by Todd Lipcon and David Alves.

Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
---
M java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
M java/kudu-client/src/test/java/org/apache/kudu/client/BaseKuduTest.java
M java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
M java/kudu-client/src/test/java/org/apache/kudu/client/TestAsyncKuduSession.java
4 files changed, 72 insertions(+), 5 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/19/4119/3
-- 
To view, visit http://gerrit.cloudera.org:8080/4119
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4119/3/java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java
File java/kudu-client/src/main/java/org/apache/kudu/client/AsyncKuduClient.java:

Line 1803:       // Make a copy so we don't need to synchronize on it while iterating.
> I really have no idea what's going on with the locking here... but it's not
yeah, same.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 1:

Forgot to add: I was under the impression that after a client reconnects, the client2tablets entry is reused. I see now that's not the case at all.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 1:

Build Started http://104.196.14.100/job/kudu-gerrit/3055/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: No

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4119/1/java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java
File java/kudu-client/src/test/java/org/apache/kudu/client/MiniKuduCluster.java:

Line 317:     for (int port : tserverPorts) {
I don't think we're managing this list very well right now. We never actually remove entries from it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 3:

Build Started http://104.196.14.100/job/kudu-gerrit/3308/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 3: Verified+1

unrelated tsan failure (org.apache.kudu.client.ITClient.test) overriding

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: David Ribeiro Alves <dr...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 2:

Build Started http://104.196.14.100/job/kudu-gerrit/3080/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No

[kudu-CR] java: fix leak of TabletClient objects in client2tablets map

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

Change subject: java: fix leak of TabletClient objects in client2tablets map
......................................................................


Patch Set 1:

erg, this is a mess... apparently we were relying on this bug somehow so that, when handling a "connection refused" from a tablet server, we were able to find all of the tablets associated with that server and "demote" the leader.

We could really use some serious refactoring/cleanup of the client, since the number of cyclic dependencies make it very hard to follow. Does anyone have any idea how to fix this?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I302650f2a6526e7c51537264137a4f00cbbda073
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No